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

LegendCategories: support for custom markers #451

Merged
merged 4 commits into from
Jul 14, 2022
Merged

LegendCategories: support for custom markers #451

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
174e53e
LegendCategories: support for custom markers.
zbigg Jul 13, 2022
82692f4
add changelog
zbigg Jul 13, 2022
97ee969
add tests
zbigg Jul 13, 2022
8da4773
cleanup css
zbigg Jul 14, 2022
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Not released

- LegendCategories: support for custom markers [#451](https://github.com/CartoDB/carto-react/pull/451)

## 1.4
## 1.3

### 1.3.0 (2022-07-11)
Expand Down
21 changes: 17 additions & 4 deletions packages/react-ui/__tests__/widgets/legend/LegendCategories.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,36 @@ describe('LegendCategories', () => {
});
test('renders colors (CARTOColors) correctly', () => {
render(<LegendCategories legend={DEFAULT_LEGEND} />);
const elements = document.querySelectorAll('[class*="circle"]');
const elements = document.querySelectorAll('[class*="markerCircle"]');
getPalette(COLOR, 2).forEach((color, idx) =>
expect(elements[idx]).toHaveStyle(`background-color: ${color}`)
);
});
test('renders colors (hex) correctly', () => {
render(<LegendCategories legend={{ ...DEFAULT_LEGEND, colors: ['#000', '#fff'] }} />);
const [firstCategory, secondCategory] =
document.querySelectorAll('[class*="circle"]');
const [firstCategory, secondCategory] = document.querySelectorAll(
'[class*="markerCircle"]'
);
expect(firstCategory).toHaveStyle('background-color: #000;');
expect(secondCategory).toHaveStyle('background-color: #fff;');
});
test('renders stroked colors correctly', () => {
render(<LegendCategories legend={{ ...DEFAULT_LEGEND, isStrokeColor: true }} />);
const elements = document.querySelectorAll('[class*="circle"]');
const elements = document.querySelectorAll('[class*="markerCircle"]');
getPalette(COLOR, 2).forEach((color, idx) =>
expect(elements[idx]).toHaveStyle(`border-color: ${color}`)
);
});
test('renders icons correctly', () => {
render(
<LegendCategories
legend={{ ...DEFAULT_LEGEND, customMarkers: 'https://xyz.com/x.png' }}
/>
);
const elements = document.querySelectorAll('[class*="markerIcon"]');
getPalette(COLOR, 2).forEach((color, idx) => {
expect(elements[idx]).toHaveStyle(`mask-image: url(https://xyz.com/x.png)`);
expect(elements[idx]).toHaveStyle(`background-color: ${color}`);
});
});
});
49 changes: 40 additions & 9 deletions packages/react-ui/src/widgets/legend/LegendCategories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { getPalette } from '../../utils/palette';
import PropTypes from 'prop-types';

function LegendCategories({ legend }) {
const { labels = [], colors = [], isStrokeColor = false } = legend;
const { labels = [], colors = [], isStrokeColor = false, customMarkers } = legend;

const palette = getPalette(colors, labels.length);

Expand All @@ -14,6 +14,9 @@ function LegendCategories({ legend }) {
isMax={false}
label={label}
color={palette[idx]}
icon={
customMarkers && Array.isArray(customMarkers) ? customMarkers[idx] : customMarkers
}
isStrokeColor={isStrokeColor}
/>
));
Expand Down Expand Up @@ -42,6 +45,10 @@ LegendCategories.propTypes = {
legend: PropTypes.shape({
labels: PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, PropTypes.number])),
colors: PropTypes.oneOfType([PropTypes.arrayOf(ColorType), PropTypes.string]),
customMarkers: PropTypes.oneOfType([
PropTypes.arrayOf(PropTypes.string),
PropTypes.string
]),
isStrokeColor: PropTypes.bool
}).isRequired
};
Expand All @@ -56,14 +63,11 @@ const useStyles = makeStyles((theme) => ({
'& $circle': {}
}
},
circle: {
marker: {
whiteSpace: 'nowrap',
display: 'block',
width: '12px',
height: '12px',
borderRadius: '50%',
position: 'relative',
border: '2px solid transparent',

'&::after': {
position: 'absolute',
display: ({ isMax }) => (isMax ? 'block' : 'none'),
Expand All @@ -76,6 +80,16 @@ const useStyles = makeStyles((theme) => ({
boxSizing: 'content-box'
}
},
markerCircle: {
width: '12px',
height: '12px',
border: '2px solid transparent',
borderRadius: '50%'
},
markerIcon: {
width: '16px',
height: '16px'
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@VictorVelarde
So update on story:

  • yes, MUI adds proper prefixes, when used in makeStyles like above!
  • but something else (react/mui?) also requires Webkit prefixes ... and filters for inline style={{...}}

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, I didn't notice those were inline styles and they were excluded. This way is better, thx!

flexParent: {
display: 'flex',
alignItems: 'center'
Expand All @@ -94,7 +108,7 @@ const useStyles = makeStyles((theme) => ({
}
}));

function Row({ label, isMax, isStrokeColor, color = '#000' }) {
function Row({ label, isMax, isStrokeColor, color = '#000', icon }) {
const classes = useStyles({ isMax });

const [showTooltip, setShowTooltip] = useState(false);
Expand All @@ -121,8 +135,25 @@ function Row({ label, isMax, isStrokeColor, color = '#000' }) {
<Box
mr={1.5}
component='span'
className={classes.circle}
style={isStrokeColor ? { borderColor: color } : { backgroundColor: color }}
className={[
classes.marker,
icon ? classes.markerIcon : classes.markerCircle
].join(' ')}
style={
icon
? {
backgroundColor: color,
maskRepeat: 'no-repeat',
maskSize: 'cover',
maskImage: `url(${icon})`,
WebkitMaskRepeat: 'no-repeat',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really required? MaterialUI is supposed to detect & add vendor prefixes (https://v4.mui.com/styles/advanced/#css-prefixes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know about mui. This is not MUI, btw, this is generic react style property ...
For sure prefixes are needed with vanilla CSS: https://jsfiddle.net/ongwmLrd/2/
Will check it...

Copy link
Contributor Author

@zbigg zbigg Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@VictorVelarde
TLDR: It is needed ...

It's some other magic mechanism, which understands Firefox
image
vs Chrome:
image

difference and ... if we don't add Webkit*, in Chrome only looks like this:

image

In other words, our "framework" understands which props are supported by which browser and emits only those into DOM.Too many frameworks for me

WebkitMaskSize: 'cover',
WebkitMaskImage: `url(${icon})`
}
: isStrokeColor
? { borderColor: color }
: { backgroundColor: color }
}
/>
</Tooltip>
<Typography ref={labelRef} variant='overline' className={classes.longTruncate}>
Expand Down