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.

Sign up for GitHub

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

Fix #3928 widget builder color selector box shows wrong labels #3937

Merged
merged 4 commits into from
Jul 12, 2019
Merged

Fix #3928 widget builder color selector box shows wrong labels #3937

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a57396a
Fix #3928 widget builder color selector box shows wrong labels
kasongoyo Jul 10, 2019
cb162c9
add tests for colorrampitem component
kasongoyo Jul 11, 2019
59f6b69
add copyright signature to the files
kasongoyo Jul 11, 2019
a2f9f44
make ColorRampItem more generic by removing locale logic
kasongoyo Jul 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 19 additions & 18 deletions web/client/components/style/EqualIntervalComponents/ColorRampItem.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,24 @@
const PropTypes = require('prop-types');
const React = require('react');
const colors = require("./ExtendColorBrewer");
const PropTypes = require('prop-types');
const colors = require('./ExtendColorBrewer');
const Message = require('../../I18N/Message');
class ColorRampItem extends React.Component {
static propTypes = {
item: PropTypes.oneOfType([PropTypes.string, PropTypes.object])
};

render() {
let ramp = this.props.item && (this.props.item.ramp || colors[this.props.item.name] && colors[this.props.item.name][5]) || [];
return (<div className="color-ramp-item">
{ramp.map(cell => <div className="color-cell" key={this.props.item && this.props.item.name + "-" + cell} style={{backgroundColor: cell}}/>)}
<div className="colorname-cell">
{this.props.item && this.props.item.name
? <Message msgId={`global.colors.${this.props.item.name}`} msgParams={{number: ramp.length}} />
: this.props.item && this.props.item.name}
</div>
</div>);
}
}
const ColorRampItem = ({ item }) => {
let ramp = item && (item.ramp || colors[item.name] && colors[item.name][5]) || [];
return (
<div className="color-ramp-item">
{ramp.map(cell => <div className="color-cell" key={item && item.name + "-" + cell} style={{ backgroundColor: cell }} />)}
<div className="colorname-cell">
{item && item.name
? <Message msgId={item.name.includes('global.colors') ? item.name : `global.colors.${item.name}`} msgParams={{ number: ramp.length }} />
Copy link
Member

@offtherailz offtherailz Jul 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think this switch makes the component less generic. This way I can not customize labels for colors (that was the original way the component worked). If name of the color (the label) is not in the global colors, you will always have an unreadable label, no way to customize it.

Is it possible instead to add global.colors outside, when the original issue was happening, and remove the global.colors string concatenation (that should be the correct way to do the fix in #3900) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. Will push the update shortly @offtherailz

: item}
</div>
</div>
);
};

ColorRampItem.propTypes = {
item: PropTypes.oneOfType([PropTypes.string, PropTypes.object])
};

module.exports = ColorRampItem;
44 changes: 44 additions & 0 deletions web/client/components/style/EqualIntervalComponents/__tests__/ColorRampItem-test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
const React = require('react');
const ReactDOM = require('react-dom');
const expect = require('expect');
const ColorRampItem = require('../ColorRampItem');

describe("Test the ColorRampItem", () => {
beforeEach((done) => {
document.body.innerHTML = '<div id="container"></div>';
setTimeout(done);
});

afterEach((done) => {
ReactDOM.unmountComponentAtNode(document.getElementById("container"));
document.body.innerHTML = '';
setTimeout(done);
});

it('creates colorramp with defaults', () => {
ReactDOM.render(<ColorRampItem />, document.getElementById("container"));
const container = document.getElementById('container');
const colorRamp = container.querySelector('.color-ramp-item');
expect(colorRamp).toExist();
});
it('colorrampitem with string item equal to blue', () => {
ReactDOM.render(<ColorRampItem item="blue" />, document.getElementById("container"));
const container = document.getElementById('container');
const colorRamp = container.querySelector('.colorname-cell');
expect(colorRamp.innerHTML).toEqual('blue');
});
it('colorrampitem with object item contain name equal to full INTL path', () => {
const color = 'global.colors.blue';
ReactDOM.render(<ColorRampItem item={{name: color}} />, document.getElementById("container"));
const container = document.getElementById('container');
const colorRamp = container.querySelector('span');
expect(colorRamp.innerHTML).toEqual(color);
});
it('colorrampitem with object item contain name without full INTL path', () => {
const color = 'blue';
ReactDOM.render(<ColorRampItem item={{name: color}} />, document.getElementById("container"));
const container = document.getElementById('container');
const colorRamp = container.querySelector('span');
expect(colorRamp.innerHTML).toEqual(`global.colors.${color}`);
});
});