ubuntu.com icon indicating copy to clipboard operation
ubuntu.com copied to clipboard

C3-713: Redesign public representation for certified machines

Open andrejvelichkovski opened this issue 1 year ago • 14 comments

Done

  • Updated the design for each certified machine on the public website:
    • Motivation: the older design was outdated and not very representative for our work
  • Implemented new view for presenting platforms. Platform is a collection of multiple machines, sharing the base components. Each platform might have more than one machine belonging to it (see example screenshots below).

The implementation is based on this design:

Screenshot from 2024-08-20 16-57-03

Demo examples

  • Machine representation: https://ubuntu-com-14135.demos.haus/certified/202212-31031
  • Platform representation: https://ubuntu-com-14135.demos.haus/certified/platforms/14169

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
  • [List additional steps to QA the new features or prove the bug has been resolved]

Issue / Card

Fixes C3-713

Screenshots

Machine page

Screenshot from 2024-08-20 14-46-06

Platform page

Screenshot from 2024-08-20 14-46-20

Help

QA steps - Commit guidelines

andrejvelichkovski avatar Aug 01 '24 09:08 andrejvelichkovski

andrejvelichkovski is not a collaborator of the repo

webteam-app avatar Aug 01 '24 09:08 webteam-app

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 69.64%. Comparing base (79b073b) to head (a9431fc). Report is 37 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14135   +/-   ##
=======================================
  Coverage   69.64%   69.64%           
=======================================
  Files         120      120           
  Lines        3419     3419           
  Branches     1178     1174    -4     
=======================================
  Hits         2381     2381           
  Misses       1013     1013           
  Partials       25       25           

codecov[bot] avatar Aug 07 '24 12:08 codecov[bot]

The test_model_details test fails because we're changing the API as well since we need more information from the server to implement the design.

nadzyah avatar Aug 14 '24 09:08 nadzyah

The demo instance link: https://ubuntu-com-14135.demos.haus/

akbarkz avatar Aug 20 '24 12:08 akbarkz

Converting this PR to draft as we are still awaiting on feedback from different partners.

andrejvelichkovski avatar Aug 22 '24 11:08 andrejvelichkovski

Glad to see it in such good shape. Just very minor comments, all for the machine pages:

  • [ ] Change the button text for the vendor link to "Visit the vendor website" to make it clear what we are doing here
  • [ ] In here, can you correctly capitalise "Notebook" in the hero H2?
  • [ ] Please only show the tabs if there's more than one release. (It's OK to still show them in the platform page, it's informative there; here it's redundant with the H2)
  • [ ] Change the second column in the two spec tables from colspan="7" to colspan="6", that way they will align nicely with the grid and the "Issues? Let us know" section.
  • [ ] To make the page lighter, I think we can do without the "Notes" heading, see below. In the end, the paragraph above is also a note. (Please also remove u-no-margin--bottom from the paragraph above)

Screenshot 2024-09-04 at 13 43 23

+1ing this already as to not block.

juanruitina avatar Sep 04 '24 11:09 juanruitina

Thanks for the review @juanruitina

  • [x] Change the button text for the vendor link to "Visit the vendor website" to make it clear what we are doing here

Sorry, I missed this, changed it now

  • [x] In here, can you correctly capitalise "Notebook" in the hero H2?

This is a bit more complicated issue. To somewhat resolve it, I added a capitalize filter that should do this. However, the root cause is that a wrong data gets exposed from our APIs. We have an open ticket to fix this, so hopefully when we get that done the data itself will be in much better shape.

  • [x] Please only show the tabs if there's more than one release. (It's OK to still show them in the platform page, it's informative there; here it's redundant with the H2)

Great suggestion, I now added an if clause to check this

  • [x] Change the second column in the two spec tables from colspan="7" to colspan="6", that way they will align nicely with the grid and the "Issues? Let us know" section.

Done.

  • [x] To make the page lighter, I think we can do without the "Notes" heading, see below. In the end, the paragraph above is also a note. (Please also remove u-no-margin--bottom from the paragraph above)

Done.

andrejvelichkovski avatar Sep 04 '24 13:09 andrejvelichkovski

@andrejvelichkovski Could you please rebase your branch with main, this will help resolve the dependency issues and hence causing error in demo

immortalcodes avatar Sep 24 '24 11:09 immortalcodes

@andrejvelichkovski Could you please rebase your branch with main, this will help resolve the dependency issues and hence causing error in demo

Thanks @immortalcodes, just rebased with main.

andrejvelichkovski avatar Sep 24 '24 12:09 andrejvelichkovski

Mostly looks good, a few nit picks:

  • generally, it's best to apply spacing at the bottom of elements, not at the top. In this case, you should use a p-section--shallow on the preceding element, rather than adding 50px margin-top. (also, 50px doesn't align to the baseline grid, it's always best to use our spacing variables instead)

image

these hrs that don't span the full width should have the class "is-muted" on them. I've asked the vanilla team to update the guidance on when to use muted horizontal rules, as I feel it is not quite clear enough currently. You ca nfind my proposed wording on this issue, soon to be added to the documentation.

  • I suggest using the 25/75 layout on medium as well, so we can keep the laptop icon and ubuntu certified badge visible on medium: before: image after: image would also apply the sdame thinking here: image

  • please remove all instances of u-sv3 and other spacing utilities and use p-section--shallow where needed. For these links specifically, I don't think it is needed. Just a muted hr in between would suffice.

  • when it comes to the pagination, is this what we expect @juanruitina? image

lyubomir-popov avatar Sep 25 '24 08:09 lyubomir-popov

Thanks a lot for the review @lyubomir-popov. I think I now addressed your concerns, but wasn't fully sure about few things.

  1. As suggested I don't hide the icon on medium sizes now. However, I had one issue that I had to define empty div column to make the top breadcrumbs fit into a 25-75 vanilla grid. I noticed that by default Vanilla orders 6 columns in medium, which can't be split into 25-75 ratio, and because of this I couldn't use the empty columns like col-start-{size}-{index}.
  2. I modified all short hrs to be muted.
  3. I removed all u-sv instances
  4. I removed the manually specified margin and used p-section--shallow as suggested.

Still, I'm a bit unsure about the margins and how everything fits together. Can you please check the updated contents on these different configurations, I think they cover all the cases the might get presented on the website:

  • Classic example: https://ubuntu-com-14135.demos.haus/certified/202111-29635
  • Classic platform example: https://ubuntu-com-14135.demos.haus/certified/platforms/12764
  • Machine with website: https://ubuntu-com-14135.demos.haus/certified/202212-30999
  • Machine with components: https://ubuntu-com-14135.demos.haus/certified/201903-26908
  • Machine with notes: https://ubuntu-com-14135.demos.haus/certified/202207-30417
  • More unique SoC machine: https://ubuntu-com-14135.demos.haus/certified/201710-25838 (also an example where we don't show the text 1 more certified configuration... below the Hardware details link)

In terms of the pagination, it was in my understanding that it is supposed to work that way, but I'm happy to change it.

Also, if you have the permissions for that, can you please re-build the demo?

andrejvelichkovski avatar Sep 25 '24 10:09 andrejvelichkovski

@immortalcodes could you complete the code and QA review here please as you previously commented.

anthonydillon avatar Sep 30 '24 14:09 anthonydillon

much better, thanks! a few final things I noticed:

  • can we please take off the margin of the tablist:

image

  • do we need to say 2-2, 3-3? why not just 2 of 4, 3 of 4

image

  • can we align the table with the content above? image

  • why u-no-padding--top? image

lyubomir-popov avatar Oct 01 '24 08:10 lyubomir-popov

Thanks for the review @lyubomir-popov. I think I have now addressed the points:

why u-no-padding--top?

I was under the impression that according to the design we want the top line of H1 to match with the top line of the category image. But this doesn't seem to be the case. I now removed this class.

can we align the table with the content above?

As we are using a custom pagination here, I have updated it to match with the Vanilla default values.

do we need to say 2-2, 3-3? why not just 2 of 4, 3 of 4

I have now updated the logic for presenting this text that if the start === end we only show one value.

can we please take off the margin of the tablist:

I have now remove the bottom margin from the list.

andrejvelichkovski avatar Oct 01 '24 11:10 andrejvelichkovski

LGTM! Thanks for showing great hardwork and resilience on this PR. I also has updated the demo. :tada:

immortalcodes avatar Nov 06 '24 05:11 immortalcodes

lgtm!

lyubomir-popov avatar Nov 06 '24 09:11 lyubomir-popov