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

feat(explore): Color scheme groups, new color schemes #27995

Merged
merged 9 commits into from
May 9, 2024

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Apr 11, 2024

SUMMARY

  • adds a bunch of new color schemes
  • groups the color schemes in categories: Custom (for color schemes added by users in config), Featured and Other
  • Adds option group handling to Select and AsyncSelect

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2024-04-11.at.18.44.54.mov

TESTING INSTRUCTIONS

  1. Test Select and AsyncSelect components with groups in storybook
  2. Add some custom color schemes in config
  3. Go to Explore and open a viz that has categorical color scheme control (for example Pie chart)
  4. Open color scheme dropdown
  5. Verify that your custom color scheme is in Custom group, the rest of the color schemes should be in groups Featured or Other

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@michael-s-molina michael-s-molina added the review:checkpoint Last PR reviewed during the daily review standup label Apr 11, 2024
@michael-s-molina
Copy link
Member

@kgabryje I have some questions about how this behavior will work with some of the requirements/features of the Select component:

  • Selected items are always sorted first. How will that work with groups?
  • We allow new items. Where will they be placed?
  • What's the interaction with Select All?

Have we considered implementing this outside of the Select component? Like a radio group that changes the values of the Select depending on the option that is selected?

If we are thinking specifically about color palettes where new values and select all are disabled, maybe this should be a specific component?

If you ask me, given the complexity involved, I would implement this with a combination of radio group + Select.

@michael-s-molina
Copy link
Member

Maybe another question would be, does it really make a difference separating color pallets by featured / others / etc? If so, another possible alternative would be to render the elements with a featured label as we do for database types.

Screenshot 2024-04-11 at 16 43 39

@kgabryje
Copy link
Member Author

kgabryje commented Apr 12, 2024

  • Selected items are always sorted first. How will that work with groups?

Good question. Currently I left it unsorted, but I was thinking about sorting the selected within the groups. The groups remain in their initial positions.

  • We allow new items. Where will they be placed?

New items do not belong to any group - they will be treated as regular (ungrouped) options

  • What's the interaction with Select All?

It simply selects all options

Have we considered implementing this outside of the Select component? Like a radio group that changes the values of the Select depending on the option that is selected?

If we are thinking specifically about color palettes where new values and select all are disabled, maybe this should be a specific component?

If you ask me, given the complexity involved, I would implement this with a combination of radio group + Select.

The option groups are supported by the native Antd Select, so in my opinion we should also support them in our customised Select. Right now the color scheme picker is our only use case for the groups, but I feel like grouping the options in categories is not such an uncommon thing to do and the need for it might pop up in the future

EDIT: formatting

@michael-s-molina
Copy link
Member

The option groups are supported by the native Antd Select, so in my opinion we should also support them in our customised Select. Right now the color scheme picker is our only use case for the groups, but I feel like grouping the options in categories is not such an uncommon thing to do and the need for it might pop up in the future

We can go down that road but keep in mind that complexity will increase given the other requirements we have and that the Ant Design select does not have such as sorting selected items and new items. If we can avoid that, it would be better. That's why I'm trying to see if there's an alternative before starting to analyze how to resolve all the questions I posted above.

@michael-s-molina
Copy link
Member

If there's no alternative, and we really need to implement this behavior, then we need to think about how to break the select component because I don't how this behavior will work with:

  • Selected items are always sorted first. How will that work with groups?
  • We allow new items. Where will they be placed?
  • What's the interaction with Select All?

@kgabryje
Copy link
Member Author

kgabryje commented Apr 12, 2024

If there's no alternative, and we really need to implement this behavior, then we need to think about how to break the select component because I don't how this behavior will work with:

  • Selected items are always sorted first. How will that work with groups?
  • We allow new items. Where will they be placed?
  • What's the interaction with Select All?

Ugh sorry, I answered those questions in my first response but messed up the formatting 🤦

Pasting it here again:

  • Selected items are always sorted first. How will that work with groups?

Good question. Currently I left it unsorted, but I was thinking about sorting the selected within the groups. The groups remain in their initial positions.

  • We allow new items. Where will they be placed?

New items do not belong to any group - they will be treated as regular (ungrouped) options

  • What's the interaction with Select All?

It simply selects/deselects all options, like with ungrouped options

@john-bodley john-bodley removed the review:checkpoint Last PR reviewed during the daily review standup label Apr 12, 2024
@michael-s-molina
Copy link
Member

@kgabryje @geido Let's do a meeting to discuss this change as I'm worried about complexity increase.

@kgabryje
Copy link
Member Author

/testenv up

Copy link
Contributor

@kgabryje Ephemeral environment spinning up at http://34.222.156.36:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@kgabryje kgabryje force-pushed the feat/color-scheme-groups branch 4 times, most recently from 6ec2feb to d110cd2 Compare April 21, 2024 15:45
@kgabryje
Copy link
Member Author

@michael-s-molina @geido Following our discussion from last week, I opted for "low effort" solution for now, which is using Antd Select for ColorSchemeControl instead of our custom Select component. We'll revisit this component in the near future and try to figure out if we can provide a better user experience by rewriting this control with different UI

@michael-s-molina
Copy link
Member

@kgabryje Did you forget to commit something? I'm still seeing some changes in the Select component such as group options.

@kgabryje
Copy link
Member Author

kgabryje commented May 8, 2024

@michael-s-molina Thanks for spotting, I updated the PR. I left the styling for groups in StyledSelect, so that the component can be reused for grouped options

@kgabryje
Copy link
Member Author

kgabryje commented May 8, 2024

/testenv up

Copy link
Contributor

github-actions bot commented May 8, 2024

@kgabryje Ephemeral environment spinning up at http://34.216.167.224:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@@ -55,6 +55,13 @@ export const StyledSelect = styled(AntdSelect, {
.select-all {
border-bottom: 1px solid ${theme.colors.grayscale.light3};
}
.ant-select-item.ant-select-item-group {
Copy link
Member

Choose a reason for hiding this comment

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

I believe these should be moved to the ColorSchemeControl:

<StyledSelect
  css={css`
    width: 100%;
    <HERE>
  `}

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

id: 'modernSunset',
label: 'Modern sunset',
group: ColorSchemeGroup.Featured,
colors: [
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 if these should go into the theme object. Could you add a TODO comment to revisit this file when working on SIP-82 Improving Superset Theming?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think all the other colour scheme files (including the old ones) should be revisited? I can add todos to all of them so that we don't miss them

Copy link
Member

Choose a reason for hiding this comment

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

That would be great! Thank you!

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for addressing the comments @kgabryje!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label May 9, 2024
@kgabryje
Copy link
Member Author

kgabryje commented May 9, 2024

/testenv up

Copy link
Contributor

github-actions bot commented May 9, 2024

@kgabryje Ephemeral environment spinning up at http://35.89.137.51:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@kgabryje kgabryje merged commit bbfe5c0 into apache:master May 9, 2024
30 checks passed
Copy link
Contributor

github-actions bot commented May 9, 2024

Ephemeral environment shutdown and build artifacts deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer packages size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants