Skip to content

Fix the top bar going outside the window#33752

Merged
gzdunek merged 1 commit intomasterfrom
gzdunek/fix-top-bar-overflow
Oct 23, 2023
Merged

Fix the top bar going outside the window#33752
gzdunek merged 1 commit intomasterfrom
gzdunek/fix-top-bar-overflow

Conversation

@gzdunek
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek commented Oct 20, 2023

Before:
image

After:
image

Changelog: Fix the top bar breaking layout when the window is narrow in Connect.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It seems unneeded - the layout is still responsive.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It technically looks better too. It's interesting that without this property, the underlying elements occupy their own minimal space, while if min-width of the parent is specified, they're squeezed inside the parent.

Here's a comparison:

Without min-width With min-width
without min-width with min-width

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Idk why 🤷‍♂️

@gzdunek gzdunek requested a review from ravicious October 20, 2023 15:13
@gzdunek gzdunek marked this pull request as ready for review October 20, 2023 15:13
@github-actions github-actions Bot requested a review from rudream October 20, 2023 15:14
@gzdunek gzdunek force-pushed the gzdunek/fix-top-bar-overflow branch from 95f59da to 207de94 Compare October 20, 2023 15:15
Copy link
Copy Markdown
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

Looks good. On Friday when we were chatting about this I brought up adding a possible scroll to accommodate people who increase the zoom level in the window (see screenshots in one of my comments) but:

  1. Technically the previous implementation didn't accommodate for zoom anyway.
  2. It's much easier to add this single min-width rather than to figure out how to deal with the scroll, and we don't have much time for the latter.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It technically looks better too. It's interesting that without this property, the underlying elements occupy their own minimal space, while if min-width of the parent is specified, they're squeezed inside the parent.

Here's a comparison:

Without min-width With min-width
without min-width with min-width

min-width: calc(${props => props.theme.space[9]}px * 2);
// min-width causes the filters and the actual input text to be broken into
// two lines when there is no space
min-width: calc(${props => props.theme.space[8]}px * 2);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume this had to be dropped from 9 to 8 to make everything fit?

I remember setting it to a specific width (but I don't remember why because I didn't leave a comment 🙃), probably for both filters to fit on a single line even when the window is at its narrowest. But at the smallest window width, it's probably better to make the top bar usable rather than to keep both filters in the same line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, when the width was 9, the content of the input was a little too wide.

But at the smallest window width, it's probably better to make the top bar usable rather than to keep both filters in the same line.

👍

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from rudream October 23, 2023 13:35
@gzdunek gzdunek enabled auto-merge October 23, 2023 14:54
@gzdunek gzdunek added this pull request to the merge queue Oct 23, 2023
Merged via the queue into master with commit a5ab44d Oct 23, 2023
@gzdunek gzdunek deleted the gzdunek/fix-top-bar-overflow branch October 23, 2023 15:21
@public-teleport-github-review-bot
Copy link
Copy Markdown

@gzdunek See the table below for backport results.

Branch Result
branch/v13 Create PR
branch/v14 Create PR

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants