ref: remove default margin from Alert in sentry #85128
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
Scope: Frontend
michellewzhang
force-pushed
the
mz/mz/update-alert-margin-sentry
branch
from
53031f9 to
83bfb04
Compare
michellewzhang
force-pushed
the
mz/mz/update-alert-margin-sentry
branch
from
dee8137 to
5e6b938
Compare
michellewzhang
force-pushed
the
mz/mz/update-alert-margin-sentry
branch
from
22b032a to
d245601
Compare
ryan953
reviewed
Feb 13, 2025
| } | ||
| > | ||
| This alert can be dismissed! | ||
| <Alert.Container> |
There was a problem hiding this comment.
It's a story i know... but this could go inside the isDismissed check so there's no "extra" space when it's dismissed
There was a problem hiding this comment.
it looks pretty much the same either way...but i updated it😆
ryan953
reviewed
Feb 13, 2025
| @@ -1,5 +1,4 @@ | |||
| import {Fragment} from 'react'; | |||
| import styled from '@emotion/styled'; | |||
There was a problem hiding this comment.
so many places that removed this import! that's a win too
ryan953
reviewed
Feb 13, 2025
Comment on lines
-201
to
-203
| const AlertWithMarginBottom = styled(Alert)` | ||
| const StyledAlert = styled(Alert)` | ||
| margin-top: ${space(2)}; | ||
| margin-bottom: 0; |
michellewzhang
commented
Feb 13, 2025
| @@ -217,10 +216,6 @@ export const DismissButton = styled(Button)` | |||
| } | |||
| `; | |||
|
|
|||
| export const StyledAlert = styled(Alert)` | |||
There was a problem hiding this comment.
this wasn't used or imported anywhere
ryan953
approved these changes
Feb 13, 2025
|
Love to see all those style imports deleted ❤️ Great idea to also target the child via > as it means the container wont take space once |
JonasBa
approved these changes
Feb 14, 2025
michellewzhang
force-pushed
the
mz/mz/update-alert-margin-sentry
branch
from
d0117d0 to
d1936e1
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Because
Alertused to have a default bottom margin, we had a lot of instances of styledAlertsacross the codebase that looked like this:The goal was to remove these instances entirely. In this PR, the default margin is removed from
Alertand placed inside aAlert.Containerexport instead.This PR also updates all
Alertinstances to eitherAlert.Containerif it was using the default margins of theAlertcomponent, ormargin: 0styleIn summary:
tldr, using
Alertgoing forwardAlertwill no longer have a default bottom margin. If you'd still like to utilize that old built-in margin, you can wrap your component with<Alert.Container>:note that components that utilize
Alertas a child, such asPanelAlertandProjectPermissionAlert, already have the<Alert.Container>wrapped around it inside the component definition.relates to #85109
see also https://github.com/getsentry/getsentry/pull/16482 (getsentry counterpart)