You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/08/10 19:06:40 UTC
[GitHub] [superset] geido opened a new pull request, #21040: feat: Add label and tooltip for the color schemes control
geido opened a new pull request, #21040:
URL: https://github.com/apache/superset/pull/21040
### SUMMARY
This PR adds the label name in the color schemes control to make it easier for the users to find their color schemes.
It also adds a tooltip when the color scheme label or the color list gets truncated.
### BEFORE
### AFTER
### TESTING INSTRUCTIONS
1. Open Explore
2. Scroll the available color schemes
3. If the label and/or the colors are truncated a tooltip should appear
4. If the label and the colors are fully visible, the tooltip should not appear
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21040:
URL: https://github.com/apache/superset/pull/21040#issuecomment-1222028926
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21040:
URL: https://github.com/apache/superset/pull/21040#issuecomment-1211585652
@stephenLYZ Ephemeral environment spinning up at http://35.91.151.185: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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on a diff in pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
geido commented on code in PR #21040:
URL: https://github.com/apache/superset/pull/21040#discussion_r948086393
##########
superset-frontend/src/explore/components/controls/ColorSchemeControl/ColorSchemeLabel.tsx:
##########
@@ -0,0 +1,124 @@
+/**
+ * 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 { css, SupersetTheme } from '@superset-ui/core';
+import React, { useRef, useState } from 'react';
+import { Tooltip } from 'src/components/Tooltip';
+
+type ColorSchemeLabelProps = {
+ colors: string[];
+ id: string;
+ label: string;
+};
+
+export default function ColorSchemeLabel(props: ColorSchemeLabelProps) {
+ const { id, label, colors } = props;
+ const [showTooltip, setShowTooltip] = useState<boolean>(false);
+ const labelNameRef = useRef<HTMLElement>(null);
+ const labelColorsRef = useRef<HTMLElement>(null);
+ const handleShowTooltip = () => {
+ const labelNameElement = labelNameRef.current;
+ const labelColorsElement = labelColorsRef.current;
+ if (
+ labelNameElement &&
+ labelColorsElement &&
+ (labelNameElement.scrollWidth > labelNameElement.offsetWidth ||
+ labelNameElement.scrollHeight > labelNameElement.offsetHeight ||
+ labelColorsElement.scrollWidth > labelColorsElement.offsetWidth ||
+ labelColorsElement.scrollHeight > labelColorsElement.offsetHeight)
+ ) {
+ setShowTooltip(true);
+ }
+ };
+ const handleHideTooltip = () => {
+ setShowTooltip(false);
+ };
+
+ const colorsList = () =>
+ colors.map((color: string, i: number) => (
+ <span
+ data-test="color"
+ key={`${id}-${i}`}
+ css={(theme: { gridUnit: number }) => css`
+ padding-left: ${theme.gridUnit / 2}px;
+ :before {
+ content: '';
+ display: inline-block;
+ background-color: ${color};
+ border: 1px solid ${color === 'white' ? 'black' : color};
+ width: 9px;
+ height: 10px;
+ }
+ `}
+ />
+ ));
+
+ const tooltipContent = () => (
+ <>
+ <span>{label}</span>
+ <div>{colorsList()}</div>
+ </>
+ );
+
+ return (
+ <Tooltip
+ data-testid="tooltip"
+ overlayClassName="color-scheme-tooltip"
+ title={tooltipContent}
+ key={id}
+ visible={showTooltip}
Review Comment:
Yes this was my first approach as well but unfortunately it won't work as it cannot evaluate the width after mount properly
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
geido commented on PR #21040:
URL: https://github.com/apache/superset/pull/21040#issuecomment-1213364515
/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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #21040:
URL: https://github.com/apache/superset/pull/21040#issuecomment-1213358449
@kasiazjc it looks like the names AND the palette colors are being truncated. Should that happen to both, or should one of them "win"?
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido merged pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
geido merged PR #21040:
URL: https://github.com/apache/superset/pull/21040
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21040:
URL: https://github.com/apache/superset/pull/21040#issuecomment-1219789684
@geido Ephemeral environment spinning up at http://34.220.222.129: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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codyml commented on a diff in pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #21040:
URL: https://github.com/apache/superset/pull/21040#discussion_r944918058
##########
superset-frontend/src/explore/components/controls/ColorSchemeControl/ColorSchemeLabel.tsx:
##########
@@ -0,0 +1,124 @@
+/**
+ * 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 { css, SupersetTheme } from '@superset-ui/core';
+import React, { useRef, useState } from 'react';
+import { Tooltip } from 'src/components/Tooltip';
+
+type ColorSchemeLabelProps = {
+ colors: string[];
+ id: string;
+ label: string;
+};
+
+export default function ColorSchemeLabel(props: ColorSchemeLabelProps) {
+ const { id, label, colors } = props;
+ const [showTooltip, setShowTooltip] = useState<boolean>(false);
+ const labelNameRef = useRef<HTMLElement>(null);
+ const labelColorsRef = useRef<HTMLElement>(null);
+ const handleShowTooltip = () => {
+ const labelNameElement = labelNameRef.current;
+ const labelColorsElement = labelColorsRef.current;
+ if (
+ labelNameElement &&
+ labelColorsElement &&
+ (labelNameElement.scrollWidth > labelNameElement.offsetWidth ||
+ labelNameElement.scrollHeight > labelNameElement.offsetHeight ||
+ labelColorsElement.scrollWidth > labelColorsElement.offsetWidth ||
+ labelColorsElement.scrollHeight > labelColorsElement.offsetHeight)
+ ) {
+ setShowTooltip(true);
+ }
+ };
+ const handleHideTooltip = () => {
+ setShowTooltip(false);
+ };
+
+ const colorsList = () =>
+ colors.map((color: string, i: number) => (
+ <span
+ data-test="color"
+ key={`${id}-${i}`}
+ css={(theme: { gridUnit: number }) => css`
+ padding-left: ${theme.gridUnit / 2}px;
+ :before {
+ content: '';
+ display: inline-block;
+ background-color: ${color};
+ border: 1px solid ${color === 'white' ? 'black' : color};
+ width: 9px;
+ height: 10px;
+ }
+ `}
+ />
+ ));
+
+ const tooltipContent = () => (
+ <>
+ <span>{label}</span>
+ <div>{colorsList()}</div>
+ </>
+ );
+
+ return (
+ <Tooltip
+ data-testid="tooltip"
+ overlayClassName="color-scheme-tooltip"
+ title={tooltipContent}
+ key={id}
+ visible={showTooltip}
Review Comment:
If I'm understanding correctly, you're explicitly setting `visible` in this case because you're measuring the component on each hover to see if a tooltip should be rendered. I don't think this is a big deal, but I'm finding that the browser can get overloaded by the listeners:
https://user-images.githubusercontent.com/13007381/184453734-762fefda-6730-4f9d-b8f4-61ffcbcb5b32.mov
You might have tried this already, but would it be possible to just measure the component once on or just after mount in a `useEffect`, and then only conditionally wrap in `Tooltip` if it's truncated and let `Tooltip` manage its own hover state without explicitly setting `visible`?
##########
superset-frontend/src/explore/components/controls/ColorSchemeControl/index.jsx:
##########
@@ -196,7 +179,7 @@ export default class ColorSchemeControl extends React.PureComponent {
};
return (
- <Select
+ <StyledSelect
Review Comment:
I'm noticing a weird thing where the selected color starts at the top of the list, but then moves to another location if you move the mouse outside the list and back in. I don't see how this PR would have caused that problem, but if it's easy to fix maybe we could fix it here?
https://user-images.githubusercontent.com/13007381/184456288-dd7dcf96-c3ed-4140-b891-5658cfa6b2fb.mov
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #21040:
URL: https://github.com/apache/superset/pull/21040#issuecomment-1211144183
# [Codecov](https://codecov.io/gh/apache/superset/pull/21040?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#21040](https://codecov.io/gh/apache/superset/pull/21040?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ad39cfb) into [master](https://codecov.io/gh/apache/superset/commit/dfe5a0493886136620ec3046d315b8d1159503d5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (dfe5a04) will **decrease** coverage by `11.63%`.
> The diff coverage is `n/a`.
> :exclamation: Current head ad39cfb differs from pull request most recent head c7b1024. Consider uploading reports for the commit c7b1024 to get more accurate results
```diff
@@ Coverage Diff @@
## master #21040 +/- ##
===========================================
- Coverage 66.26% 54.62% -11.64%
===========================================
Files 1769 1769
Lines 67470 67470
Branches 7170 7170
===========================================
- Hits 44708 36857 -7851
- Misses 20931 28782 +7851
Partials 1831 1831
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `53.05% <ø> (ø)` | |
| python | `57.41% <ø> (-24.05%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `50.49% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/21040?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...e/components/controls/ColorSchemeControl/index.jsx](https://codecov.io/gh/apache/superset/pull/21040/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Db2xvclNjaGVtZUNvbnRyb2wvaW5kZXguanN4) | `63.63% <ø> (ø)` | |
| [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/21040/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/21040/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-88.89%)` | :arrow_down: |
| [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/21040/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-85.30%)` | :arrow_down: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/21040/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.19%)` | :arrow_down: |
| [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/21040/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-80.77%)` | :arrow_down: |
| [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/21040/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <0.00%> (-76.25%)` | :arrow_down: |
| [superset/datasets/commands/update.py](https://codecov.io/gh/apache/superset/pull/21040/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvdXBkYXRlLnB5) | `25.00% <0.00%> (-69.05%)` | :arrow_down: |
| [superset/datasets/commands/create.py](https://codecov.io/gh/apache/superset/pull/21040/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvY3JlYXRlLnB5) | `29.41% <0.00%> (-68.63%)` | :arrow_down: |
| [superset/datasets/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/21040/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvaW1wb3J0ZXJzL3YwLnB5) | `24.03% <0.00%> (-67.45%)` | :arrow_down: |
| ... and [281 more](https://codecov.io/gh/apache/superset/pull/21040/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kasiazjc commented on pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
kasiazjc commented on PR #21040:
URL: https://github.com/apache/superset/pull/21040#issuecomment-1218216263
> > Thanks for the feedback Cody! I think actually that is a good idea. We still need an indicator that not all of the colors are shown and I'm thinking - maybe let's add a counter? So, as in the categorical color palette, max number of colors is 12, let's use it's a max shown. It will be the fixed with for the color swatches section. Then between text and colors there is 8px fixed spacing and the rest is title. Does it make sense? All of the things would cut off in the same space and it would declutter the whole thing a little bit. What do you think @geido
> > <img alt="image" width="879" src="https://user-images.githubusercontent.com/36897697/184870687-a675e98b-2071-408a-bea4-0f6267502f1a.png">
>
> @kasiazjc the two elements cannot be located at the opposite ends as the container can be dragged and expanded. This would create a potential huge space between the label and the colors.
>
> In order to keep this flexible we also should not set a maximum number of 12 colors as the dropdown should behave based on the available horizontal space.
>
> As for changing from ellipsis to a number of hidden colors, that would require a bigger refactor and if we think it is critical we should discuss that option and I can open another PR.
>
> In the meantime, I made some enhancements so that the options do not look disordered and all consume the same space now, which should satisfy what @codyml was asking above. Also, These latest changes truncates both the labels and colors less often, which should improve the feelings about double truncation.
>
> 
Oh, I understand. I actually think it looks good! I am totally fine with merging this version. Thank you Diego for working on it 🙏
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21040:
URL: https://github.com/apache/superset/pull/21040#issuecomment-1213367720
@geido Ephemeral environment spinning up at http://52.33.179.163: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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] stephenLYZ commented on pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
stephenLYZ commented on PR #21040:
URL: https://github.com/apache/superset/pull/21040#issuecomment-1211584517
/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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on a diff in pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
geido commented on code in PR #21040:
URL: https://github.com/apache/superset/pull/21040#discussion_r948088270
##########
superset-frontend/src/explore/components/controls/ColorSchemeControl/index.jsx:
##########
@@ -196,7 +179,7 @@ export default class ColorSchemeControl extends React.PureComponent {
};
return (
- <Select
+ <StyledSelect
Review Comment:
This is a Select component issue that it makes sense to fix in a separate PR. I'll open an issue for this problem. Thanks for catching it!
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
geido commented on PR #21040:
URL: https://github.com/apache/superset/pull/21040#issuecomment-1213365323
> @kasiazjc it looks like the names AND the palette colors are being truncated. Should that happen to both, or should one of them "win"?
The proposed design truncates both. I believe it makes sense since you can have a very large amount of colors or a very lenghty name for the palette.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kasiazjc commented on pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
kasiazjc commented on PR #21040:
URL: https://github.com/apache/superset/pull/21040#issuecomment-1216520438
> Code looks good and it works! Left two comments that are not blocking. The double truncation does strike me as kind of strange: the row of swatches starts at a different place for each item so the menu looks a little chaotic, and as a user I think my first impression of seeing two ellipses in one menu item would be that there's a layout bug. I can't think of a perfect alternative, but what about if we made it so we had a max of like 10 swatches and they were right-justified without an ellipsis, and then the title is left-justified, truncated with an ellipsis always at the same point? We wouldn't see the max amount of information possible but it would look a lot cleaner and the tooltips would show the full content.
>
> <img alt="Screen Shot 2022-08-12 at 5 43 57 PM" width="667" src="https://user-images.githubusercontent.com/13007381/184457442-67066c0e-e62f-4af1-bf94-902136b75c8f.png">
>
> But, also non-blocking if we want to stick with this design.
Thanks for the feedback Cody! I think actually that is a good idea. We still need an indicator that not all of the colors are shown and I'm thinking - maybe let's add a counter? So, as in the categorical color palette, max number of colors is 12, let's use it's a max shown. It will be the fixed with for the color swatches section. Then between text and colors there is 8px fixed spacing and the rest is title. Does it make sense? All of the things would cut off in the same space and it would declutter the whole thing a little bit. What do you think @geido
<img width="879" alt="image" src="https://user-images.githubusercontent.com/36897697/184870687-a675e98b-2071-408a-bea4-0f6267502f1a.png">
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
geido commented on PR #21040:
URL: https://github.com/apache/superset/pull/21040#issuecomment-1218184308
> Thanks for the feedback Cody! I think actually that is a good idea. We still need an indicator that not all of the colors are shown and I'm thinking - maybe let's add a counter? So, as in the categorical color palette, max number of colors is 12, let's use it's a max shown. It will be the fixed with for the color swatches section. Then between text and colors there is 8px fixed spacing and the rest is title. Does it make sense? All of the things would cut off in the same space and it would declutter the whole thing a little bit. What do you think @geido
>
> <img alt="image" width="879" src="https://user-images.githubusercontent.com/36897697/184870687-a675e98b-2071-408a-bea4-0f6267502f1a.png">
@kasiazjc the two elements cannot be located at the opposite ends as the container can be dragged and expanded. This would create a potential huge space between the label and the colors.
In order to keep this flexible we also should not set a maximum number of 12 colors as the dropdown should behave based on the available horizontal space.
As for changing from ellipsis to a number of hidden colors, that would require a bigger refactor and if we think it is critical, we should do explore it more and I can open another PR.
In the meantime, I made some enhancements so that the options do not look disordered and all consume the same space, which should satisfy what @codyml was asking above. These latest changes truncates both the labels and colors less often, which should improve the feelings about double truncation.
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] geido commented on pull request #21040: feat: Add label and tooltip for the color schemes control
Posted by GitBox <gi...@apache.org>.
geido commented on PR #21040:
URL: https://github.com/apache/superset/pull/21040#issuecomment-1219785826
/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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org