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

Move the "remove" button to the sidebar footer #38050

Merged
merged 8 commits into from
Jan 25, 2024

Conversation

oleggromov
Copy link
Contributor

@oleggromov oleggromov commented Jan 23, 2024

Epic: #36524

Description

This is one of the obvious steps in the 2nd milestone of the epic.
The remove button obviously doesn't belong to where it was so we're moving it to the bottom of the sidebar.

Since I've touched the Sidebar, I am also converting it to Typescript and updating styles.

Before

Screenshot 2024-01-24 at 15 11 57

After

Screenshot 2024-01-25 at 11 39 31

@oleggromov oleggromov self-assigned this Jan 23, 2024
@oleggromov oleggromov requested a review from a team January 23, 2024 17:37
@oleggromov oleggromov added the no-backport Do not backport this PR to any branch label Jan 23, 2024
Copy link

replay-io bot commented Jan 23, 2024

StatusComplete ↗︎
Commit0a901f3
Results
⚠️ 3 Flaky
  • should allow non-user to unsubscribe from subscription
  • should convert 'is empty' on a text column to a custom expression using IsEmpty()
  • should validate approved email domains for a dashboard subscription in the audit app
2234 Passed

Copy link
Contributor

@kamilmielnik kamilmielnik left a comment

Choose a reason for hiding this comment

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

👍

@oleggromov oleggromov force-pushed the required-params-dashboard-button branch from 035ed0c to a69e950 Compare January 24, 2024 13:24
@oleggromov oleggromov force-pushed the required-params-dashboard-button branch from 66c4759 to e3e6404 Compare January 25, 2024 11:37
variant="subtle"
color="error"
onClick={onRemove}
style={{ paddingLeft: 0, paddingRight: 0 }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we're probably awaiting a transition from mantine 6 to 7, I am adding an inline style, which will be replaced with a CSS class once migrated.

@oleggromov oleggromov changed the title WIP Move the "remove" button to the sidebar footer Move the "remove" button to the sidebar footer Jan 25, 2024
@oleggromov oleggromov merged commit 6955751 into master Jan 25, 2024
106 checks passed
@oleggromov oleggromov deleted the required-params-dashboard-button branch January 25, 2024 12:52
Copy link

@oleggromov Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

sloansparger pushed a commit that referenced this pull request Feb 5, 2024
* Convert Sidebar to TSX
* Move remove button to the sidebar footer
* Change styles to styled components
* Fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/QueryingComponents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants