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

LegendCategories: support for custom markers #451

Merged

Conversation

zbigg
Copy link
Contributor

@zbigg zbigg commented Jul 13, 2022

Description

Shortcut: https://app.shortcut.com/cartoteam/story/244132/implement-support-for-markers-in-builder-i-single-icon

Add support for rendering of 'masked colored icons' in LegendCategories widget instead of classic circles.

Rationale: This feature is needed in builder and it's simpler to implement here as we already use LegendCategories as a whole.

Needed by: https://github.com/CartoDB/cloud-native/pull/8559

image

Type of change

(choose one and remove the others)

  • Feature

Acceptance

Basic checklist

  • Good PR name
  • Shortcut link
  • Changelog entry
  • Just one issue per PR
  • GitHub labels
  • Proper status & reviewers
  • Tests
  • Documentation

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #244132: Implement support for markers in Builder (I): Single icon.

@zbigg zbigg force-pushed the feature/sc-244132/implement-support-for-markers-in-builder branch 2 times, most recently from c1b2df7 to f8e32bb Compare July 13, 2022 14:00
@zbigg zbigg force-pushed the feature/sc-244132/implement-support-for-markers-in-builder branch from f8e32bb to 174e53e Compare July 13, 2022 14:28
@coveralls
Copy link
Collaborator

coveralls commented Jul 13, 2022

Pull Request Test Coverage Report for Build 2669328841

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 72.051%

Totals Coverage Status
Change from base Build 2651180346: 0.04%
Covered Lines: 1606
Relevant Lines: 2087

💛 - Coveralls

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

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

@VictorVelarde VictorVelarde changed the title LegendCategories: support for custom markers (experiment). LegendCategories: support for custom markers Jul 13, 2022
icon: {
maskRepeat: 'no-repeat',
maskSize: 'cover'
},
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!

@zbigg zbigg force-pushed the feature/sc-244132/implement-support-for-markers-in-builder branch from 9aaa4ee to 8da4773 Compare July 14, 2022 09:27
@zbigg zbigg requested a review from VictorVelarde July 14, 2022 09:27
Copy link
Contributor

@padawannn padawannn left a comment

Choose a reason for hiding this comment

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

LGFM

@zbigg zbigg merged commit e9ec25d into master Jul 14, 2022
@zbigg zbigg deleted the feature/sc-244132/implement-support-for-markers-in-builder branch July 14, 2022 11:02
@zbigg zbigg mentioned this pull request Jul 14, 2022
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.

4 participants