Change 921402 had a related patch set uploaded (by Viviana Yanez; author: Viviana Yanez):

[mediawiki/extensions/GrowthExperiments@master] Frontend documentation: Implement a prototype of OnboardingDialog pattern latest spec

https://gerrit.wikimedia.org/r/921402

Change 922478 had a related patch set uploaded (by Viviana Yanez; author: Viviana Yanez):

[mediawiki/extensions/GrowthExperiments@master] Frontend documentation: Remove unused less variables and styles

https://gerrit.wikimedia.org/r/922478

Change 922496 had a related patch set uploaded (by Viviana Yanez; author: Viviana Yanez):

[mediawiki/extensions/GrowthExperiments@master] Frontend documentation: actualize documentation for OnboardingDialog

https://gerrit.wikimedia.org/r/922496

Change 922478 abandoned by Viviana Yanez:

[mediawiki/extensions/GrowthExperiments@master] Frontend documentation: Update less variables and OnboardingDialog styles

Reason:

https://gerrit.wikimedia.org/r/922478

Change 922478 restored by Viviana Yanez:

[mediawiki/extensions/GrowthExperiments@master] Frontend documentation: Update less variables and OnboardingDialog styles

https://gerrit.wikimedia.org/r/922478

Change 921402 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Frontend documentation: Implement a prototype of OnboardingDialog pattern latest spec

https://gerrit.wikimedia.org/r/921402

Change 922478 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Frontend documentation: Update less variables and OnboardingDialog styles

https://gerrit.wikimedia.org/r/922478

Change 922496 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Frontend documentation: Update documentation for OnboardingDialog

https://gerrit.wikimedia.org/r/922496

Change 924889 had a related patch set uploaded (by Viviana Yanez; author: Viviana Yanez):

[mediawiki/extensions/GrowthExperiments@master] Frontend documentation: update dialog styles

https://gerrit.wikimedia.org/r/924889

Change 924889 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Frontend documentation: update dialog styles

https://gerrit.wikimedia.org/r/924889

We're missing some visual minor issues:

• Stepper font-size
• Fixed size in the onboarding dialog demo ( height: 520px; ) and add a documentation comment.
• Header height
• Review overflow content borders
• ...

Change 933492 had a related patch set uploaded (by Viviana Yanez; author: L10n-bot):

[mediawiki/extensions/GrowthExperiments@master] Frontend documentation: fix visual issues in onboarding dialog styles

https://gerrit.wikimedia.org/r/933492

Change 933492 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Frontend documentation: fix visual issues in onboarding dialog styles

https://gerrit.wikimedia.org/r/933492

Moving this task to QA to give a chance to @bmartinezcalvo to perform a final review of the pattern implementation in the demo site. One mistake I've observed is the font-size for the checkbox is 16px rather than 14px in desktop. Since it is a minor fix I will amend it within the work in T329037.

@Sgs thank you, I've reviewed the demo and the pattern looks great. Just listing here some corrections I've detected:

1. Stepper dots should be @size-50 (8px)
2. Padding between dialog's title and stepper is @spacing-75 (12px) (in the spec is specified 6px but then there is another 6px within the text box)
3. Padding between bodytext and link in the step 2 should be @spacing-50( 8px)
4. The image heigh should use our system sizing tokens so it should be @size-1600 (256px) height on desktop and tablet and @size-800 (128px) height on mobile
5. Text styles. There are some text styles to fix since they are mixing the base sizes from desktop and mobile:
   • "Skip all" button is using a 16px text size while "Get started" is using 14px text size. Both should be 14px on desktop and 16px on mobile
   • Checkbox text size should be @font-size-small (14px) on desktop and @font-size-medium (16px) on mobile, following the base text sizes on both platforms
   • Dialog's title on mobile should be @font-size-large (18px) text size. While in desktop is 16px because the base text size is 14px.

Regarding the "Don't show again", is the intention to display it in all the steps or just in the first one?

Change 936667 had a related patch set uploaded (by Sergio Gimeno; author: Sergio Gimeno):

[mediawiki/extensions/GrowthExperiments@master] Onboarding dialog: update styles to match spec

https://gerrit.wikimedia.org/r/936667

In T336270#8997597, @bmartinezcalvo wrote:

@Sgs thank you, I've reviewed the demo and the pattern looks great. Just listing here some corrections I've detected:

Thanks for reviewing. Something I missed to explain is the VitePress docs site as a base font of 16px on desktop so the demos should be reviewed as "in mobile". I've added comments between lines on the action taken for each issue you found.

1. Stepper dots should be @size-50 (8px)

This specification seems particularly problematic because of the fixed-relative units problem mentioned above. I've added a comment about it in {T333584#9000653 }

2. Padding between dialog's title and stepper is @spacing-75 (12px) (in the spec is specified 6px but then there is another 6px within the text box)

Fixed

3. Padding between bodytext and link in the step 2 should be @spacing-50( 8px)

Fixed

4. The image heigh should use our system sizing tokens so it should be @size-1600 (256px) height on desktop and tablet and @size-800 (128px) height on mobile

We don't have assets that match the specification you are mentioning and we're adjusting the already existing ones

5. Text styles. There are some text styles to fix since they are mixing the base sizes from desktop and mobile:
   • "Skip all" button is using a 16px text size while "Get started" is using 14px text size. Both should be 14px on desktop and 16px on mobile

Fixed. We're using @font-size-medium as the base font-size for the dialog

• Checkbox text size should be @font-size-small (14px) on desktop and @font-size-medium (16px) on mobile, following the base text sizes on both platforms

Fixed by the fix above. Just a clarification, the statement above is incorrect. The token to use is the same on both platforms: @font-size-medium.

• Dialog's title on mobile should be @font-size-large (18px) text size. While in desktop is 16px because the base text size is 14px.

Fixed

Regarding the "Don't show again", is the intention to display it in all the steps or just in the first one?

In all steps.

Reflecting on this I think we should just do a final review in the already integrated dialog in MW rather than doing QA on the VitePress site because of the environment differences.

I wonder if we should specify padding with relative units (em, rem) rather than fixed (px). It seems more aligned with https://www.mediawiki.org/wiki/Manual:Coding_conventions/CSS#Sizing_units although I might be wrong. What do you think @bmartinezcalvo @Volker_E?

In T336270#9000764, @Sgs wrote:

1. Stepper dots should be @size-50 (8px)

This specification seems particularly problematic because of the fixed-relative units problem mentioned above. I've added a comment about it in {T333584#9000653 }

@Sgs commenting here the same as in the other task: as specified in the Figma spec, the Stepper will use 14px font size on both desktop and mobile platforms. So the dots will also be the same 8px size on both platforms.

I wonder if we should specify padding with relative units (em, rem) rather than fixed (px). It seems more aligned with https://www.mediawiki.org/wiki/Manual:Coding_conventions/CSS#Sizing_units although I might be wrong. What do you think @bmartinezcalvo @Volker_E?

I'm not really sure why Size tokens use relative units while Spacing not. @Catrope @AnneT @egardner is there a specific reason why we are not using relative units for paddings?

Change 936667 abandoned by Sergio Gimeno:

[mediawiki/extensions/GrowthExperiments@master] Onboarding dialog: update styles to match spec

Reason:

Squashed in Ia041b1502aafeb89881f2f34051845758bc698aa to avoid an extra bundle build before the the first roll out

https://gerrit.wikimedia.org/r/936667

In T336270#9000983, @bmartinezcalvo wrote:

I'm not really sure why Size tokens use relative units while Spacing not. @Catrope @AnneT @egardner is there a specific reason why we are not using relative units for paddings?

It's for when the user increases the font size in their browser. When that happens, we want sizes to scale, but we don't want spacing to scale.

Change 931300 had a related patch set uploaded (by Urbanecm; author: Sergio Gimeno):

[mediawiki/extensions/GrowthExperiments@master] Frontend documentation: add missing instrumentation events

https://gerrit.wikimedia.org/r/931300

Judging based on open patches that this is still to be finished. Moving off (old) sprint. @Sgs, please feel free to correct me!