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

Randomize color scheme order #47304

Closed
wants to merge 1 commit into from

Conversation

mreishus
Copy link
Contributor

Changes proposed in this Pull Request

  • Color scheme order on /me/account is randomized.
    • Purpose: To eliminate location in list as a popularity variable when analyzing data.
    • Lifetime: This method of randomization seems to be per-(server process). Is that ok? My previous method of just adding a shuffle() call changed the order every time you clicked, with hilarious results.
    • Awkward mechanics of this change are to preserve the translation dependency injection while randomizing a list such that it doesn't change every time you click.

Testing instructions

  • Visit /me/account
  • See a color scheme order
  • Stop yarn and restart
  • Repeat and see a different color scheme order
    2020-11-10_17-54

Reference Issues

@matticbot
Copy link
Contributor

@mreishus mreishus self-assigned this Nov 10, 2020
@mreishus mreishus requested a review from a team November 10, 2020 23:54
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 10, 2020
@matticbot
Copy link
Contributor

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

Webpack Runtime (~24 bytes added 📈 [gzipped])

name      parsed_size           gzip_size
manifest        +77 B  (+0.1%)      +24 B  (+0.2%)

Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded.

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

name        parsed_size           gzip_size
entry-main        +23 B  (+0.0%)       +2 B  (+0.0%)

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

Sections (~408 bytes added 📈 [gzipped])

name     parsed_size           gzip_size
account       +931 B  (+0.2%)     +408 B  (+0.4%)

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 (~220 bytes added 📈 [gzipped])

name                      parsed_size           gzip_size
async-load-design-blocks       +855 B  (+0.0%)     +183 B  (+0.0%)
async-load-design              +185 B  (+0.0%)      +37 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.

Copy link
Contributor

@frontdevde frontdevde left a comment

Choose a reason for hiding this comment

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

I tested as per testing instructions and the color schemes feature behaved the following way:
When navigating to /me/account, then navigating somewhere else on the site without a reload, then navigating to /me/account again, the selection stayed the same. When navigating to /me/account and then reloading the same page, the selection got shuffled around. I assume this is the wanted behavior, right?

Aside: I understand the idea that it'll help make tracking data less dependent on the order of the color schemes but I'm personally not sure this leads to a better user experience. That's just my personal opinion though, it's up to design/product to decide what's best for the user.

One thing I did notice is that we might have lost the translations in the process? See:

Production This branch
Screenshot 2020-11-11 at 12 56 52 Screenshot 2020-11-11 at 12 56 57

@mreishus
Copy link
Contributor Author

Ok, since the dependency injection fix didn't work, I'm abandoning this.

@mreishus mreishus closed this Nov 11, 2020
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 11, 2020
@mreishus
Copy link
Contributor Author

#47342 Display in alphabetical order

@mreishus mreishus deleted the add/randomized-order-color-schemes branch November 11, 2022 18:50
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.

3 participants