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

Improved chart names for chart type dropdown. #7631

Merged
merged 7 commits into from
Oct 10, 2019
Merged

Conversation

smartguest
Copy link
Contributor

Added to SelectBox a map of alternative names that are user friendly for charts.
Dropdown bar displays alternative names while not physically altering the chart objects themselves.

Fixes #38 (preliminary)

@smartguest smartguest merged commit 4d618c5 into master Oct 10, 2019
Copy link
Contributor

@aaomidi aaomidi left a comment

Choose a reason for hiding this comment

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

This technically works - but the select box component shouldn't have to be modified just for a label change.

//Map used to store names and alternative names for chart types.
//This is mainly used for comparison when options are parsed into the constructor.
const altNameHash : {[oldName: string]: string} = {
['horizontalBar']: 'Horziontal Bar',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this an array mapped to a string? Why not a string to string?

constructor(options: string[], selectedOption: string, contextViewProvider: IContextViewProvider, container?: HTMLElement, selectBoxOptions?: ISelectBoxOptions) {
super(options.map(option => { return { text: option }; }), 0, contextViewProvider, undefined, selectBoxOptions);
//originally {text :option };
super(options.map(option => {return {text : SelectBox.parseName(option)}; }), 0, contextViewProvider, undefined, selectBoxOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

This creates a weird cyclic dependency. A component shouldn't know what's going into it - if there are general rules its applying it should be for everything rather than a specific usage of it.

For example, say this was a "Hot Water" component, a "Hot Water" component doesn't need to know its going to be used for "Tea" or "Coffee". It just needs to know how to behave on its own.

Essentially you should be doing this mapping right before it comes in here, rather than on the component itself.

Another thing you could do is extend this class and make a special case class for graph SelectBoxes - but even then if the only behavior that is different is the name, we should just pass that as a property.

@aaomidi aaomidi requested a review from kburtram October 10, 2019 18:14
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.

Chart Type values are not user friendly
2 participants