Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: refactor Alert-related components #31858

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

chore: refactor Alert-related components #31858

wants to merge 1 commit into from
+571 −726

Conversation

mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Jan 15, 2025 •
edited
Loading

Chiseling at #31590 that has gotten big / unruly, in this PR is a refactor of Alert-related components, going vanilla AntD.

Also here. Deprecating colors.alerts since it's ambiguous/redundant with warning/error and does not exist in antd-v5

Screenshot 2025-01-14 at 11 42 05 PM Screenshot 2025-01-14 at 11 41 47 PM

@korbit-ai Korbit AI
Copy link

korbit-ai bot commented Jan 15, 2025

Korbit doesn't automatically review large (500+ lines changed) pull requests such as this one. If you want me to review anyway, use /korbit-review.

@dosubot dosubot bot added the alert-reports Namespace | Anything related to the Alert & Reports feature label Jan 15, 2025
@msyavuz msyavuz mentioned this pull request Jan 15, 2025
Open
9 tasks
@geido
Copy link
Member

geido commented Jan 15, 2025

From some functional testing:

  • A bit of an edge case but the error looks like it is not correctly padded in full page width
Screenshot 2025-01-15 at 17 26 57
  • Is a chart failure considered a warning?
Screenshot 2025-01-15 at 17 28 55
  • Same, for a chart in Explore. I am thinking these should probably be errors.
Screenshot 2025-01-15 at 17 30 49

  • The positioning of the icon on the left of the text looks off.

  • In general, the text color of these alerts seem to stick out a bit too much. I know we want be as vanilla as possible but I'd like to hear @kasiazjc opinion too.

geido
geido reviewed Jan 15, 2025
View reviewed changes
superset-frontend/src/components/ErrorMessage/ErrorAlert.stories.tsx
component: ErrorAlert,
} as Meta;

export const Gallery: StoryFn = () => (
Copy link
Member

Choose a reason for hiding this comment

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

This is not coming with this PR but I find it odd that an ErrorAlert can be of type info or warning. Naming here could probably be improved.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, maybe limiting to warning/error here makes more sense here. I think there may be one or two edge cases where ErrorAlert is used to info, maybe cause someone wanted some of the features in here. We should probably change those to use a normal Alert

geido
geido reviewed Jan 15, 2025
View reviewed changes
superset-frontend/src/components/ErrorMessage/ErrorAlert.tsx Outdated
title: ReactNode;
description?: string;
import { useState } from 'react';
import { Modal, Tooltip } from 'antd';
Copy link
Member

Choose a reason for hiding this comment

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

Why importing from antd directly? From one side this is the previous version of antd and on the other hand, I think we said to import components from our wrappers not from antd directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

ooops. We really need a lint rule to catch this...

@justinpark justinpark self-requested a review January 15, 2025 18:25
mistercrunch
mistercrunch commented Jan 15, 2025
View reviewed changes
superset-frontend/src/theme/index.ts
@@ -56,15 +56,6 @@ const baseConfig: ThemeConfig = {
zIndexPopupBase: supersetTheme.zIndex.max,
},
components: {
Alert: {
Copy link
Member Author

Choose a reason for hiding this comment

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

Let's go vanilla!

@github-actions github-actions bot added the doc Namespace | Anything related to documentation label Jan 16, 2025
@github-actions github-actions bot removed the doc Namespace | Anything related to documentation label Jan 16, 2025
@mistercrunch mistercrunch force-pushed the alerts_refactor branch 3 times, most recently from c5c85ea to b96705d Compare January 18, 2025 00:26
@github-actions github-actions bot added the github_actions Pull requests that update GitHub Actions code label Jan 19, 2025
@mistercrunch mistercrunch force-pushed the alerts_refactor branch 2 times, most recently from 219ef36 to 40bad64 Compare January 21, 2025 18:12
@github-actions github-actions bot removed the github_actions Pull requests that update GitHub Actions code label Jan 21, 2025
@sadpandajoe
Copy link
Member

/testenv up

@github-actions GitHub Actions
Copy link
Contributor

@sadpandajoe Processing your ephemeral environment request here.

@sadpandajoe
Copy link
Member

@kasiazjc hoping to spin up an ephemeral for you to get a pass on this if you have time.

@github-actions GitHub Actions
Copy link
Contributor

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

@mistercrunch
chore: refactor Alert-related components
37768e7 
Chiseling at #31590 that has gotten big / unruly, in this PR is a refactor of Alert-related components, going vanilla AntD.

Also. Deprecating colors.alerts since it's ambiguous/redundant with warning/error and does not exist in antd-v5
geido
geido reviewed Jan 22, 2025
View reviewed changes
superset-frontend/src/components/Alert/index.tsx
)
}
closeIcon={closable && <Icons.XSmall aria-label="close icon" />}
closeIcon={closable}
Copy link
Member

Choose a reason for hiding this comment

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

Should this be closable instead of closeIcon given that the value of closable is a boolean and that of closeIcon is supposed to be a ReactNode?

superset-frontend/src/components/ErrorMessage/ErrorAlert.tsx
};
const preStyle = {
whiteSpace: 'pre-wrap',
// fontFamily: theme.antd.fontFamilyCode,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// fontFamily: theme.antd.fontFamilyCode,

geido
geido requested changes Jan 22, 2025
View reviewed changes
Copy link
Member

@geido geido left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There was a E2E failing and this brought me to check SQL Lab which seems broken on this branch. Checked on master and could not repro there.

Screenshot 2025-01-22 at 15 36 23

Also not sure what the other Cypress failures are about, first time seeing this error on our CI:

Error: Failed to CreateArtifact: Received non-retryable error: Failed request: (409) Conflict: an artifact with this name already exists on the workflow run

I have been looking at other E2E tests and I did not see any failures strictly related to this work so I think we'll be good here once that issue on SQL Lab is fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geido geido geido requested changes

@justinpark justinpark Awaiting requested review from justinpark

@sadpandajoe sadpandajoe Awaiting requested review from sadpandajoe

@eschutho eschutho Awaiting requested review from eschutho

@rusackas rusackas Awaiting requested review from rusackas

@betodealmeida betodealmeida Awaiting requested review from betodealmeida

@villebro villebro Awaiting requested review from villebro

@nytai nytai Awaiting requested review from nytai

@craig-rueda craig-rueda Awaiting requested review from craig-rueda

@kgabryje kgabryje Awaiting requested review from kgabryje

@dpgaspar dpgaspar Awaiting requested review from dpgaspar

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
alert-reports Namespace | Anything related to the Alert & Reports feature packages plugins preset-io size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mistercrunch @geido @sadpandajoe