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.

Sign up for GitHub

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

Jump to bottom

Wd 8751 update the pro subscribe page visuals #13601

Merged
merged 10 commits into from
Mar 11, 2024
Merged

Wd 8751 update the pro subscribe page visuals #13601

merged 10 commits into from
Mar 11, 2024

Conversation

abhigyanghosh30
Copy link
Contributor

@abhigyanghosh30 abhigyanghosh30 commented Feb 21, 2024 •
edited by jira bot
Loading

Done

  • updated /pro/subscribe page

QA

  • Check out this feature branch
  • Run the site using the command ./run serve or dotrun
  • View the site locally in your web browser at: http://0.0.0.0:8001/
    • Be sure to test on mobile, tablet and desktop screen sizes
  • checkout /pro/subscribe
  • Should match the visuals here

Issue / Card

Fixes #WD-8751

Screenshots

image

Help

QA steps - Commit guidelines

@webteam-app
Copy link

Demo starting at https://ubuntu-com-13601.demos.haus

@jpmartinspt
Copy link
Contributor

@lyubomir-popov mind doing a design review on this one please?

@codecov Codecov
Copy link

codecov bot commented Feb 25, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.49%. Comparing base (7508b69) to head (7dd83cb).
Report is 149 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #13601      +/-   ##
==========================================
+ Coverage   74.41%   74.49%   +0.07%     
==========================================
  Files         107      107              
  Lines        2838     2854      +16     
  Branches      946      954       +8     
==========================================
+ Hits         2112     2126      +14     
- Misses        702      704       +2     
  Partials       24       24
Files Coverage Δ
...ubscribe/react/components/Form/Feature/Feature.tsx 85.71% <100.00%> (+10.71%) ⬆️
...react/components/ProductSummary/ProductSummary.tsx 92.30% <ø> (ø)

... and 2 files with indirect coverage changes

@leiaru-c
Copy link

A couple of things I noticed:

  • On smaller screens, there is an addition of two blocks of text referring to the amount of packages in the different repositories that are not present on bigger screens. I believe they are redundant and should not be added at all. I would suggest to remove the section highlighted in yellow on the following screenshot.

Screenshot_newshopbug1

  • The floating bar at the bottom breaks between around 628 and 1026 in width, as shown in the following screenshots:

Screenshot_newshopbug2
Screenshot_newshopbug2_2

  • On smaller screens the horizontal alignment of the logos is odd, I think it would look more balanced to have the logos aligned centre to avoid uneven spacing as shown in the following screenshots:

Screenshot_newshopbug3
Screenshot_newshopbug4

jpmartinspt
jpmartinspt reviewed Mar 1, 2024
View reviewed changes
Copy link
Contributor

@jpmartinspt jpmartinspt left a comment

Choose a reason for hiding this comment

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

A few things to change after talking to @lechsandecki. We might want to update the copydoc too to reflect the changes.

static/js/src/advantage/subscribe/react/components/Form/Feature/Feature.tsx Outdated
/>
<hr />
<RadioInput
label="Infra only - limited subset"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change this to Infra - only limited subset.

static/js/src/advantage/subscribe/react/components/Form/Feature/Feature.tsx Outdated
</Row>
<Row>
<Col size={2} emptyMedium={2} emptyLarge={2}>
<h1>{feature === Features.pro ? "25000" : "2300"}</h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

25,000 and 2,300

static/js/src/advantage/subscribe/react/components/Form/Feature/Feature.tsx Outdated
<Col size={3}>
<p>
Covers {feature === Features.pro ? "25000" : "2300"} packages in
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole thing can become just packages covered since we have the descriptions on the graph labels.

static/js/src/advantage/subscribe/react/components/Form/Feature/Feature.tsx Outdated
disabled={infraOnlyDisabled}
/>
<p className="p-text--small graphic-legend-main">
LTS standard security maintenance for Ubuntu Main (initial 5 years)
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to 10 years of security maintenance for packages in Ubuntu Main repository

static/js/src/advantage/subscribe/react/components/Form/Feature/Feature.tsx Outdated
</p>
{feature === Features.pro && (
<p className="p-text--small graphic-legend-universe">
LTS Expanded Security Maintenance (ESM) for Ubuntu Main
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to 10 years of security maintenance for packages in Ubuntu Universe repository

jpmartinspt
jpmartinspt reviewed Mar 8, 2024
View reviewed changes
static/js/src/advantage/subscribe/react/components/Form/Feature/Feature.tsx Outdated
<Col size={3}>
<p>
Covers {feature === Features.pro ? "25,000" : "2,300"} packages in
Copy link
Contributor

Choose a reason for hiding this comment

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

This one slipped through the first round of commends you can remove the Covers {feature === Features.pro ? "25,000" : "2,300"}.

jpmartinspt
jpmartinspt approved these changes Mar 8, 2024
View reviewed changes
Copy link
Contributor

@jpmartinspt jpmartinspt left a comment

Choose a reason for hiding this comment

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

Approving with a change requested.

@abhigyanghosh30 abhigyanghosh30 merged commit 6e36f97 into canonical:main Mar 11, 2024
15 checks passed
@abhigyanghosh30 abhigyanghosh30 deleted the WD-8751-update-the-pro-subscribe-page-visuals branch March 11, 2024 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpmartinspt jpmartinspt jpmartinspt approved these changes

@lyubomir-popov lyubomir-popov Awaiting requested review from lyubomir-popov

Assignees
No one assigned
Labels
Review: Code +1 (with changes) Review: QA +1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@abhigyanghosh30 @webteam-app @jpmartinspt @leiaru-c