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/07/05 23:13:21 UTC
[GitHub] [superset] codyml opened a new pull request, #20612: chore(explore): update Explore icons and icon colors
codyml opened a new pull request, #20612:
URL: https://github.com/apache/superset/pull/20612
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR changes the info and error icons on the Explore screen to AntD icons, and makes the icons blue instead of red when required fields aren't filled.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
Before:
https://user-images.githubusercontent.com/13007381/177431595-f6d571b7-61b6-44c3-baf6-95070012b824.mov
After:
https://user-images.githubusercontent.com/13007381/177431630-3f901310-c0c5-4588-b1bc-d7496268470b.mov
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
- Confirm that other than icon and icon color changes, nothing has changed.
- Confirm that no old-style icons are left that I missed on the Explore screen.
### 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] rusackas commented on a diff in pull request #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
rusackas commented on code in PR #20612:
URL: https://github.com/apache/superset/pull/20612#discussion_r933365353
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -531,14 +588,23 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
placement="right"
title={props.errorMessage}
>
- <i className="fa fa-exclamation-circle text-danger fa-lg" />
Review Comment:
I can't tell you how happy it makes me to see these fa-* icons fading away one by one :D
--
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 #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20612:
URL: https://github.com/apache/superset/pull/20612#discussion_r914884881
##########
superset-frontend/src/explore/components/ControlHeader.tsx:
##########
@@ -78,12 +82,13 @@ const ControlHeader: FC<ControlHeaderProps> = ({
>
{description && (
<span>
- <InfoTooltipWithTrigger
Review Comment:
It's still used on line 96 to render the lightning bolt icon, do you think I should replace that with a plain `<Tooltip>...</Tooltip>` as well?
--
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 pull request #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #20612:
URL: https://github.com/apache/superset/pull/20612#issuecomment-1176274955
> Hi @codyml.
> IMHO blue color uses in info icons but a required field alway uses red color to better notice it
The motivation for this was to not show red right away on new charts because it can make the user think they've done something wrong right from the beginning, which isn't great for user experience. But, I'm going to darken the blue color, which hopefully will make it a little more visible, and I'm also going to try to change it so it's only blue if they've never added an item to a required field on a new chart, and after that if they clear a required field on a new or existing chart it'll show the error in red. Does that sound like a good improvement?
--
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 #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #20612:
URL: https://github.com/apache/superset/pull/20612#issuecomment-1198866568
/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] EugeneTorap commented on pull request #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
EugeneTorap commented on PR #20612:
URL: https://github.com/apache/superset/pull/20612#issuecomment-1175750547
Hi @codyml.
IMHO blue color uses in info icons but a required field alway uses red color to better notice 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] codyml commented on a diff in pull request #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20612:
URL: https://github.com/apache/superset/pull/20612#discussion_r927173925
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -58,6 +58,11 @@ import { ChartState, ExplorePageState } from 'src/explore/types';
import { Tooltip } from 'src/components/Tooltip';
import { rgba } from 'emotion-rgba';
+import { kebabCase } from 'lodash';
+import {
+ ExclamationCircleOutlined,
+ InfoCircleOutlined,
+} from '@ant-design/icons';
Review Comment:
Oops thanks! I've done that a couple of other times, I can open another PR that fixes the others.
--
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 #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20612:
URL: https://github.com/apache/superset/pull/20612#issuecomment-1199622972
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] geido commented on a diff in pull request #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
geido commented on code in PR #20612:
URL: https://github.com/apache/superset/pull/20612#discussion_r927627567
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -58,6 +58,11 @@ import { ChartState, ExplorePageState } from 'src/explore/types';
import { Tooltip } from 'src/components/Tooltip';
import { rgba } from 'emotion-rgba';
+import { kebabCase } from 'lodash';
+import {
+ ExclamationCircleOutlined,
+ InfoCircleOutlined,
+} from '@ant-design/icons';
Review Comment:
If those are not too many, since we have to change them here, it might be a good idea to change them in this PR but I am happy to change the others in a separate PR as long as we fix the ones in this PR
--
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 #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20612:
URL: https://github.com/apache/superset/pull/20612#discussion_r930432343
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -58,6 +58,11 @@ import { ChartState, ExplorePageState } from 'src/explore/types';
import { Tooltip } from 'src/components/Tooltip';
import { rgba } from 'emotion-rgba';
+import { kebabCase } from 'lodash';
+import {
+ ExclamationCircleOutlined,
+ InfoCircleOutlined,
+} from '@ant-design/icons';
Review Comment:
Done – had to add a `font-size: unset;` to uses because the `Icons` component defaults to `font-size: 24px;`.
--
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 #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20612:
URL: https://github.com/apache/superset/pull/20612#discussion_r927731112
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -58,6 +58,11 @@ import { ChartState, ExplorePageState } from 'src/explore/types';
import { Tooltip } from 'src/components/Tooltip';
import { rgba } from 'emotion-rgba';
+import { kebabCase } from 'lodash';
+import {
+ ExclamationCircleOutlined,
+ InfoCircleOutlined,
+} from '@ant-design/icons';
Review Comment:
No worries I'll do it here!
--
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] EugeneTorap commented on a diff in pull request #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
EugeneTorap commented on code in PR #20612:
URL: https://github.com/apache/superset/pull/20612#discussion_r914388987
##########
superset-frontend/src/explore/components/ControlHeader.tsx:
##########
@@ -78,12 +82,13 @@ const ControlHeader: FC<ControlHeaderProps> = ({
>
{description && (
<span>
- <InfoTooltipWithTrigger
Review Comment:
Remove `InfoTooltipWithTrigger` import in this file
--
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 merged pull request #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
rusackas merged PR #20612:
URL: https://github.com/apache/superset/pull/20612
--
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 #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20612:
URL: https://github.com/apache/superset/pull/20612#issuecomment-1175608432
# [Codecov](https://codecov.io/gh/apache/superset/pull/20612?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 [#20612](https://codecov.io/gh/apache/superset/pull/20612?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (aed584b) into [master](https://codecov.io/gh/apache/superset/commit/818962cc89aad34afdb8ea673908416d99631a06?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (818962c) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head aed584b differs from pull request most recent head 4223fe1. Consider uploading reports for the commit 4223fe1 to get more accurate results
```diff
@@ Coverage Diff @@
## master #20612 +/- ##
=======================================
Coverage 66.82% 66.82%
=======================================
Files 1752 1752
Lines 65609 65610 +1
Branches 6938 6938
=======================================
+ Hits 43842 43843 +1
Misses 20007 20007
Partials 1760 1760
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.85% <100.00%> (+<0.01%)` | :arrow_up: |
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/20612?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...-frontend/src/explore/components/ControlHeader.tsx](https://codecov.io/gh/apache/superset/pull/20612/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Db250cm9sSGVhZGVyLnRzeA==) | `83.33% <100.00%> (ø)` | |
| [.../src/explore/components/ControlPanelsContainer.tsx](https://codecov.io/gh/apache/superset/pull/20612/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9Db250cm9sUGFuZWxzQ29udGFpbmVyLnRzeA==) | `80.18% <100.00%> (+0.18%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/20612?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/20612?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [818962c...4223fe1](https://codecov.io/gh/apache/superset/pull/20612?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] michael-s-molina commented on pull request #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20612:
URL: https://github.com/apache/superset/pull/20612#issuecomment-1179250390
When looking at this screen, I feel that if I click on `Metric` it will trigger an action just like all the other blue stuff on the screen. I understand the reasoning behind the change, I'm just not sure if the color or how we are highlighting can be improved. @kasiazjc @rusackas
<img width="1779" alt="Screen Shot 2022-07-08 at 3 09 59 PM" src="https://user-images.githubusercontent.com/70410625/178047624-b8f95f35-d0cf-4fc4-a16f-bff1781f25ca.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] codyml commented on a diff in pull request #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20612:
URL: https://github.com/apache/superset/pull/20612#discussion_r930432343
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -58,6 +58,11 @@ import { ChartState, ExplorePageState } from 'src/explore/types';
import { Tooltip } from 'src/components/Tooltip';
import { rgba } from 'emotion-rgba';
+import { kebabCase } from 'lodash';
+import {
+ ExclamationCircleOutlined,
+ InfoCircleOutlined,
+} from '@ant-design/icons';
Review Comment:
Done – had to add a `font-size: unset;` to uses because the `Icons` component defaults to `font-size: 24px;`.
--
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 #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20612:
URL: https://github.com/apache/superset/pull/20612#issuecomment-1198867553
@rusackas Ephemeral environment spinning up at http://52.26.8.229: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] rusackas commented on a diff in pull request #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
rusackas commented on code in PR #20612:
URL: https://github.com/apache/superset/pull/20612#discussion_r933364148
##########
superset-frontend/src/explore/components/ControlHeader.tsx:
##########
@@ -40,6 +40,16 @@ export type ControlHeaderProps = {
danger?: string;
};
+const iconStyles = css`
Review Comment:
This is fine for now, but let's talk about this chunk in person sometime. I'd love to be in a state where we don't need to be adding/overriding/tweaking Icon styles in any particular use/implementation, instead adding classes/config/props on the Icon component itself to handle these needs more globally.
--
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 pull request #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #20612:
URL: https://github.com/apache/superset/pull/20612#issuecomment-1185837914
> When looking at this screen, I feel that if I click on `Metric` it will trigger an action just like all the other blue stuff on the screen. I understand the reasoning behind the change, I'm just not sure if the color or how we are highlighting can be improved. @kasiazjc @rusackas
>
> <img alt="Screen Shot 2022-07-08 at 3 09 59 PM" width="1779" src="https://user-images.githubusercontent.com/70410625/178047624-b8f95f35-d0cf-4fc4-a16f-bff1781f25ca.png">
@michael-s-molina @kasiazjc Implemented design feedback: color updated to be blue and the label of the required field (e.g. "Metrics") stays black.
--
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 #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
geido commented on code in PR #20612:
URL: https://github.com/apache/superset/pull/20612#discussion_r926821106
##########
superset-frontend/src/explore/components/ControlHeader.tsx:
##########
@@ -55,6 +59,18 @@ const ControlHeader: FC<ControlHeaderProps> = ({
danger,
}) => {
const { gridUnit, colors } = useTheme();
+ const hasHadNoErrors = useRef(false);
+ const labelColor = useMemo(() => {
+ if (!validationErrors.length) {
+ hasHadNoErrors.current = true;
+ }
+
+ return hasHadNoErrors.current
Review Comment:
We tend to avoid doing this. It would be better to unpack this logic for better readability
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -58,6 +58,11 @@ import { ChartState, ExplorePageState } from 'src/explore/types';
import { Tooltip } from 'src/components/Tooltip';
import { rgba } from 'emotion-rgba';
+import { kebabCase } from 'lodash';
+import {
+ ExclamationCircleOutlined,
+ InfoCircleOutlined,
+} from '@ant-design/icons';
Review Comment:
We don't import icons directly from Antdesign. You need to use the _Icons_ component that comes with Antdesign icons built-in as well
--
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 #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
geido commented on code in PR #20612:
URL: https://github.com/apache/superset/pull/20612#discussion_r926823160
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -58,6 +58,11 @@ import { ChartState, ExplorePageState } from 'src/explore/types';
import { Tooltip } from 'src/components/Tooltip';
import { rgba } from 'emotion-rgba';
+import { kebabCase } from 'lodash';
+import {
+ ExclamationCircleOutlined,
+ InfoCircleOutlined,
+} from '@ant-design/icons';
Review Comment:
We don't import icons directly from Antdesign. You need to use the `Icons` component that comes with Antdesign icons built-in as well
--
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 #20612: chore(explore): update Explore icons and icon colors
Posted by GitBox <gi...@apache.org>.
codyml commented on code in PR #20612:
URL: https://github.com/apache/superset/pull/20612#discussion_r930442234
##########
superset-frontend/src/explore/components/ControlPanelsContainer.tsx:
##########
@@ -58,6 +58,11 @@ import { ChartState, ExplorePageState } from 'src/explore/types';
import { Tooltip } from 'src/components/Tooltip';
import { rgba } from 'emotion-rgba';
+import { kebabCase } from 'lodash';
+import {
+ ExclamationCircleOutlined,
+ InfoCircleOutlined,
+} from '@ant-design/icons';
Review Comment:
Actually, it turns out the directly-imported AntD icons render different HTML/styles from the `Icons`-rendered ones: there's an extra wrapper element and it resets the font size. I found 10 or so instances of directly importing AntD icons and swapping them out caused a bunch of layout bugs. I need to look into this further to try to fix the layout bug you found above, because I think that may be caused by my inconsistently switching to direct AntD import in some places in this PR, but I think switching them out across the codebase will be a more involved process that might be better done in a different PR.
--
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