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 i18n picker layout #56485

Merged
merged 3 commits into from
Sep 23, 2021
Merged

Fix i18n picker layout #56485

merged 3 commits into from
Sep 23, 2021

Conversation

noahtallen
Copy link
Member

@noahtallen noahtallen commented Sep 23, 2021

Changes proposed in this Pull Request

The i18n picker layout can become broken. I noticed that language picker is written with flex components from @wordpress/components. That package was updated from v13.x to v17.x in the React 17 PR. I notice this line in the changelog from the v14 release: Flex, FlexBlock, and FlexItem components have been re-written from the ground up (#31297).

Looking at the rendered markup for the language picker, it looked like it was getting incorrect values for flex-grow and flex-direction. From the source code, it looks like the default value for expanded is true and direction is set to row. Since we do not set these props, the layout was broken. After setting them to false and column, the layout appears to be fixed.

Also, I had to override the type for the flex component because definitely typed is out of date. I was unable to fix it by using yarn patches, or by removing definitely typed. So we might have to do more work in the future to properly resolve the type problem.

resolves #56372

Screenshot from 2021-09-22 18-43-46

Screenshot from 2021-09-22 18-44-20

Testing instructions

  1. Load calypso.live.
  2. Open the language picker under me/account. Change the language.
  3. After the page refreshes, the language picker should still look okay. Previously, the layout would become broken after the redirect.
  4. Open gutenboarding /new. Open the language picker at the top right of the page.
  5. The layout should look correct. Previously, the layout was broken.

see #56372 for examples of what it looked like broken.

@noahtallen noahtallen self-assigned this Sep 23, 2021
@github-actions
Copy link

github-actions bot commented Sep 23, 2021

@matticbot matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 23, 2021
@matticbot
Copy link
Contributor

matticbot commented Sep 23, 2021

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~16 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-gutenboarding        +33 B  (+0.0%)      +16 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~75 bytes added 📈 [gzipped])

name      parsed_size           gzip_size
settings        +33 B  (+0.0%)      +23 B  (+0.0%)
media           +33 B  (+0.0%)      +22 B  (+0.0%)
account         +33 B  (+0.0%)      +30 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~45 bytes added 📈 [gzipped])

name                                               parsed_size           gzip_size
async-load-quick-language-switcher                       +33 B  (+0.0%)      +23 B  (+0.0%)
async-load-design-playground                             +33 B  (+0.0%)      +22 B  (+0.0%)
async-load-design-blocks                                 +33 B  (+0.0%)      +22 B  (+0.0%)
async-load-design                                        +33 B  (+0.0%)      +22 B  (+0.0%)
async-load-calypso-post-editor-media-modal               +33 B  (+0.0%)      +22 B  (+0.0%)
async-load-calypso-post-editor-editor-media-modal        +33 B  (+0.0%)      +22 B  (+0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@scinos
Copy link
Contributor

scinos commented Sep 23, 2021

Nice catch!

Do we know why it failed only some times?

@Robertght
Copy link

Robertght commented Sep 23, 2021

I've tested it and it looks ok.

The only thing I'm not sure of is the layout on a very wide screen. Should it be considered a max-width perhaps?

image

@zdenys
Copy link
Contributor

zdenys commented Sep 23, 2021

Tested. Looks good to me. Nice catch by @Robertght though!

@noahtallen
Copy link
Member Author

Do we know why it failed only some times?

Honestly, I have no idea! The default props should mean that the layout is broken all the time. I kind of wonder if the very first time you open it, it's loading a cached version from a while ago, and then after the refresh, it loads the new version??

From what I can tell, gutenboarding is broken all of the time, just not the account switcher

The only thing I'm not sure of is the layout on a very wide screen. Should it be considered a max-width perhaps?

Nice, I'll come up with a fix

@noahtallen
Copy link
Member Author

Styles now look like:

Screenshot from 2021-09-23 10-39-24

Screenshot from 2021-09-23 10-39-09

@noahtallen noahtallen merged commit e9bea6d into trunk Sep 23, 2021
@noahtallen noahtallen deleted the fix/i18n-picker branch September 23, 2021 18:37
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 23, 2021
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.

Modal window for site language in Settings > General > Language has undesired scrollbars
5 participants