Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
github-actions[bot] commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2102805345 Ephemeral environment shutdown and build artifacts deleted. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
kgabryje merged PR #27995: URL: https://github.com/apache/superset/pull/27995 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
github-actions[bot] commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2102659301 @kgabryje Ephemeral environment spinning up at http://35.89.137.51:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
kgabryje commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2102635734 /testenv up -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
michael-s-molina commented on code in PR #27995:
URL: https://github.com/apache/superset/pull/27995#discussion_r1595396552
##
superset-frontend/packages/superset-ui-core/src/color/colorSchemes/categorical/modernSunset.ts:
##
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import CategoricalScheme from '../../CategoricalScheme';
+import { ColorSchemeGroup } from '../../types';
+
+const schemes = [
+ {
+id: 'modernSunset',
+label: 'Modern sunset',
+group: ColorSchemeGroup.Featured,
+colors: [
Review Comment:
That would be great! Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
kgabryje commented on code in PR #27995:
URL: https://github.com/apache/superset/pull/27995#discussion_r1595395284
##
superset-frontend/packages/superset-ui-core/src/color/colorSchemes/categorical/modernSunset.ts:
##
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import CategoricalScheme from '../../CategoricalScheme';
+import { ColorSchemeGroup } from '../../types';
+
+const schemes = [
+ {
+id: 'modernSunset',
+label: 'Modern sunset',
+group: ColorSchemeGroup.Featured,
+colors: [
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
kgabryje commented on code in PR #27995:
URL: https://github.com/apache/superset/pull/27995#discussion_r1595389127
##
superset-frontend/packages/superset-ui-core/src/color/colorSchemes/categorical/modernSunset.ts:
##
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import CategoricalScheme from '../../CategoricalScheme';
+import { ColorSchemeGroup } from '../../types';
+
+const schemes = [
+ {
+id: 'modernSunset',
+label: 'Modern sunset',
+group: ColorSchemeGroup.Featured,
+colors: [
Review Comment:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
kgabryje commented on code in PR #27995:
URL: https://github.com/apache/superset/pull/27995#discussion_r1595388869
##
superset-frontend/src/components/Select/styles.tsx:
##
@@ -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 {
Review Comment:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
michael-s-molina commented on code in PR #27995:
URL: https://github.com/apache/superset/pull/27995#discussion_r1595376768
##
superset-frontend/src/components/Select/styles.tsx:
##
@@ -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 {
Review Comment:
I believe these should be moved to the `ColorSchemeControl`:
```
`}
```
##
superset-frontend/packages/superset-ui-core/src/color/colorSchemes/categorical/modernSunset.ts:
##
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import CategoricalScheme from '../../CategoricalScheme';
+import { ColorSchemeGroup } from '../../types';
+
+const schemes = [
+ {
+id: 'modernSunset',
+label: 'Modern sunset',
+group: ColorSchemeGroup.Featured,
+colors: [
Review Comment:
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](https://github.com/apache/superset/issues/20159)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
github-actions[bot] commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2100831228 @kgabryje Ephemeral environment spinning up at http://34.216.167.224:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
kgabryje commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2100808166 /testenv up -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
kgabryje commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2100791678 @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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
michael-s-molina commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2075358354 @kgabryje Did you forget to commit something? I'm still seeing some changes in the Select component such as group options. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
kgabryje commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2069681858 @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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
github-actions[bot] commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2063843248 @kgabryje Ephemeral environment spinning up at http://34.222.156.36:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
kgabryje commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2063825153 /testenv up -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
michael-s-molina commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2052203399 @kgabryje @geido Let's do a meeting to discuss this change as I'm worried about complexity increase. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
kgabryje commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2051935402 > 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 all options -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
michael-s-molina commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2051917287 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? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
michael-s-molina commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2051911763 > 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
kgabryje commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2051848575 > * 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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
michael-s-molina commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2050403617 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. https://github.com/apache/superset/assets/70410625/5e0360ec-4ece-4d95-833e-990a5973e8c7;> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] feat(explore): Color scheme groups, new color schemes [superset]
michael-s-molina commented on PR #27995: URL: https://github.com/apache/superset/pull/27995#issuecomment-2050385763 @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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
