C3-713: Redesign public representation for certified machines
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:
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 serveordotrun - 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
Platform page
Help
andrejvelichkovski is not a collaborator of the repo
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
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.
The demo instance link: https://ubuntu-com-14135.demos.haus/
Converting this PR to draft as we are still awaiting on feedback from different partners.
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"tocolspan="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--bottomfrom the paragraph above)
+1ing this already as to not block.
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"tocolspan="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--bottomfrom the paragraph above)
Done.
@andrejvelichkovski Could you please rebase your branch with main, this will help resolve the dependency issues and hence causing error in demo
@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.
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)
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:
after:
would also apply the sdame thinking here:
-
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?
Thanks a lot for the review @lyubomir-popov. I think I now addressed your concerns, but wasn't fully sure about few things.
- 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}. - I modified all short hrs to be muted.
- I removed all
u-svinstances - 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?
@immortalcodes could you complete the code and QA review here please as you previously commented.
much better, thanks! a few final things I noticed:
- can we please take off the margin of the tablist:
- do we need to say 2-2, 3-3? why not just 2 of 4, 3 of 4
-
can we align the table with the content above?
-
why u-no-padding--top?
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.
LGTM! Thanks for showing great hardwork and resilience on this PR. I also has updated the demo. :tada:
lgtm!