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

[docs-infra] Remove random layout assignment #40862

Merged
merged 5 commits into from
Feb 14, 2024

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Jan 30, 2024

API page component can get props to:

  • define what is the default layout
  • define which localeStorage key is saving the layout

This should allow to have a specific layout for data grid and save it in distinct key/value. Plus it's flexible since you can customize that at the page level.

The GA behavior is also modified. We stop the experiement so no need to compare the default and current layout.
However it can be interesting in the future to know what people are using per page. So the toggle button send an event to google analytics to know when user modify the layout

See for why: https://groups.google.com/a/mui.com/g/docs-infra/c/bzqriS4u3_M/m/8Me75UBAAgAJ. The layout shifts, it's not a great page load experience.

Preview: https://deploy-preview-40862--material-ui.netlify.app/material-ui/api/alert/

Works as expected. If you go on pickers API page you get a table layout, and on data grid API page you get the list layout 🎉

@alexfauquette alexfauquette added the scope: docs-infra Specific to the docs-infra product label Jan 30, 2024
@mui-bot
Copy link

mui-bot commented Jan 30, 2024

Netlify deploy preview

https://deploy-preview-40862--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against f5f1a19

Comment on lines -142 to -147
React.useEffect(() => {
window.gtag('set', 'user_properties', {
...getApiPageLayout(),
});
}, []);

Copy link
Member

Choose a reason for hiding this comment

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

We could keep GA properties if the default layout is changed?

I would want to see who is going for:

if the adoption is low, e.g. 5% I would be in favor of removing, reducing mental overhead.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could keep GA properties if the default layout is changed?

The current GA behavior is to store layout as user settings. which was a requirement because for the same user we wanted to track the randomly assigned layout and the chosen one.

Since this requirement does not exist now, I propose to move the GA to a system where we track modification instead of the saved state. Each time a user click on one of those button, it send an GA event.

I would want to see who is going for collapsed

Me too and that's par of the reason. I use it when I'm searching for a specific props. And this toggle of collapsed/expanded is for now not visible in our data collected because we only store one state per user. It should be visible in the new one since we will listen at all clicks

Copy link
Member

Choose a reason for hiding this comment

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

Since this requirement does not exist now, I propose to move the GA to a system where we track modification instead of the saved state. Each time a user click on one of those button, it send an GA event.

I don't know if we can rely on this data. I have tried a bit https://analytics.google.com/analytics/web/?authuser=1#/analysis/p353089763/edit/6jil-1NpQv-a3sMCL6YFlw

SCR-20240319-bjbs

but how to judge people trying one option out and revert to the ones who stays on it?

@@ -64,10 +67,12 @@ export default function ClassesSection(props: ClassesSectionProps) {
level: Level = 'h2',
displayClassKeys,
styleOverridesLink,
defaultLayout,
layoutStorageKey = API_LAYOUT_STORAGE_KEYS.classes,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should have the default values for layoutStorageKey defined in ApiPage. I imagine if we override them, we want people to override them for the whole page.

@alexfauquette alexfauquette marked this pull request as ready for review February 7, 2024 10:38
@alexfauquette
Copy link
Member Author

What about adding this piece of code to clean the localeStorage from the experiment?

function cleanLayoutStorage() {
  const defaultLayout = localStorage.getItem('apiPage_default');
  if (defaultLayout === null) {
    return;
  }
  // Those can come from to the random experiement
  localStorage.removeItem('apiPage_slots');
  localStorage.removeItem('apiPage_props');
  localStorage.removeItem('apiPage_classes');
  localStorage.removeItem('apiPage_css');

  localStorage.removeItem('apiPage_default');
}

Copy link
Contributor

@danilo-leal danilo-leal 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 to me — no remarks on my end! 🤙

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: docs-infra Specific to the docs-infra product
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants