Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SliceColor helper #1239

Merged
merged 3 commits into from
Jan 18, 2025

Conversation

michaelchadwick
Copy link
Contributor

Helps clean up redundant code to determine the slice color in Bar, Donut, HorzBar, and Pie chart types

Copy link

netlify bot commented Jan 17, 2025

Deploy Preview for ember-simple-charts ready!

Name Link
🔨 Latest commit 5791312
🔍 Latest deploy log https://app.netlify.com/sites/ember-simple-charts/deploys/678af9925ed0230008decd2f
😎 Deploy Preview https://deploy-preview-1239--ember-simple-charts.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@jrjohnson jrjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great cleanup!

@@ -0,0 +1,13 @@
import { helper } from '@ember/component/helper';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this needs to be a "helper". Could just be a plain function in src/utils/slice-color.js

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there more overhead to it being a "helper"? I assumed this was more of the Ember way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The helper part usually means it's wrapped up in some kind of code to make it usable in a template. I don't really know exactly what that is. Want to dig into it a bit and see what you turn up? I'm ok to merge as is. Your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From https://discuss.emberjs.com/t/best-practice-what-helpers-might-do/14431/2?u=michaelchadwick:

A helper should generally be a simple function that returns a value that is meant to be used in a template.

I suppose it is overkill to have it in a helper when it's not used in the template, so I'll refactor.

@michaelchadwick michaelchadwick merged commit 7e6fc3e into ilios:master Jan 18, 2025
16 checks passed
@michaelchadwick michaelchadwick deleted the add-slice-color-helper branch January 18, 2025 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants