Skip to content

fix(ui): restore height for pageFilterBar #93800

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

Conversation

TkDodo
Copy link
Contributor

@TkDodo TkDodo commented Jun 18, 2025

before after
Screenshot 2025-06-18 at 13 51 14 Screenshot 2025-06-18 at 13 51 08

@JonasBa we used to have the height explicitly set - like it’s also done in the old theme - but it was removed here:

https://github.com/getsentry/sentry/pull/90243/files#diff-32f8334cfccf64122985ca68a14eb551d05ece05ea1e4f3390974d3765195a03L170

do you remember why it was removed, and is it safe to bring it back like this?

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jun 18, 2025
@TkDodo TkDodo requested a review from JonasBa June 18, 2025 12:02
@TkDodo TkDodo marked this pull request as ready for review June 18, 2025 12:02
@@ -168,6 +168,8 @@ except in mobile */
display: flex;
position: relative;

height: ${p.theme.form.md.height};
Copy link
Member

Choose a reason for hiding this comment

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

They shouldn't ever get as large as in the initial screenshot, so I wonder if there might be some custom CSS rule that could be removed? In theory, if that is fixed, I would expect the size to drop down to the size of the rest of the buttons in that row, and so this height might not even be needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn’t find anything :/

@TkDodo TkDodo merged commit 05a5e9b into master Jun 18, 2025
46 checks passed
@TkDodo TkDodo deleted the tkdodo/fix/de-153-fat-button-issue-in-mobile-vitals-screen-summary branch June 18, 2025 15:34
Copy link

codecov bot commented Jun 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #93800      +/-   ##
==========================================
- Coverage   88.02%   84.32%   -3.70%     
==========================================
  Files       10332    10332              
  Lines      596373   596373              
  Branches    23163    23163              
==========================================
- Hits       524968   502919   -22049     
- Misses      70949    92998   +22049     
  Partials      456      456              

andrewshie-sentry pushed a commit that referenced this pull request Jun 19, 2025
| before | after |
|--------|--------|
| ![Screenshot 2025-06-18 at 13 51
14](https://github.com/user-attachments/assets/434cca79-25d5-4321-b99d-dbce0633bd8b)
| ![Screenshot 2025-06-18 at 13 51
08](https://github.com/user-attachments/assets/b037f6b5-8870-4d56-876f-8955d6c0c90f)
|

@JonasBa we used to have the `height` explicitly set - like it’s also
done in the old theme - but it was removed here:


https://github.com/getsentry/sentry/pull/90243/files#diff-32f8334cfccf64122985ca68a14eb551d05ece05ea1e4f3390974d3765195a03L170

do you remember why it was removed, and is it safe to bring it back like
this?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants