Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix button for large pipeline warning #1428

Merged
merged 2 commits into from
Jun 30, 2023

Conversation

rashidakanchwala
Copy link
Contributor

@rashidakanchwala rashidakanchwala commented Jun 30, 2023

Description

This is to fix this issue

Screenshot 2023-06-29 at 17 41 31

Development notes

This bug was introduced due to changes on this ticket - #875 where display:flex is being added here. I have removed it for now. Removing this hasn't affected the Apply/Close buttons but @tynandebold can confirm.

Signed-off-by: Rashida Kanchwala <[email protected]>
Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

Left a comment about how removing the display: flex changes something we don't want it to. Also, I think the buttons should be next to each other, like they are in our modals:

image

@@ -35,7 +35,6 @@ $secondary-underline-offset-hover: 4px;
box-shadow: none;
color: var(--color-button__text);
cursor: pointer;
display: flex;
Copy link
Member

Choose a reason for hiding this comment

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

We need this, otherwise it alters the Apply changes and close button:

Screenshot 2023-06-30 at 11 45 58

Signed-off-by: Rashida Kanchwala <[email protected]>
Copy link
Member

@tynandebold tynandebold left a comment

Choose a reason for hiding this comment

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

LGTM!

<Button mode="secondary" onClick={onDisable} size="small">
Don't show this again
</Button>
<div className="pipeline-warning__button-wrapper">
Copy link
Contributor

Choose a reason for hiding this comment

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

hey @rashidakanchwala Do we not need any styling for this new classname ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, this div basically ensures that the buttons are not next to each other ... just followed this way (https://github.com/kedro-org/kedro-viz/blob/main/src/components/settings-modal/settings-modal.js#L149)

Copy link
Contributor

@Huongg Huongg left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@rashidakanchwala rashidakanchwala merged commit 7f7ed8f into main Jun 30, 2023
@rashidakanchwala rashidakanchwala deleted the fix-large-pipeline-warnings-button branch June 30, 2023 12:39
@tynandebold tynandebold mentioned this pull request Jul 7, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants