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

Feat: Rich vertical list pattern #5306

Draft
wants to merge 25 commits into
base: main
Choose a base branch
from
Draft

Feat: Rich vertical list pattern #5306

wants to merge 25 commits into from

Conversation

jmuzina
Copy link
Member

@jmuzina jmuzina commented Aug 16, 2024

Done

Builds macro, examples, and documentation for the rich vertical list pattern per design

Fixes WD-14038

QA

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix release (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

Screenshots

Screenshot from 2024-08-16 14-57-46
Screenshot from 2024-08-16 14-57-53
Screenshot from 2024-08-16 14-58-03

@jmuzina jmuzina added Feature 🎁 New feature or request Don't merge labels Aug 16, 2024
@webteam-app
Copy link

@jmuzina jmuzina changed the title wip: Feat: Initial rich vertical list wip: Feat: Rich vertical list pattern Aug 16, 2024
@jmuzina jmuzina changed the title wip: Feat: Rich vertical list pattern Feat: Rich vertical list pattern Aug 16, 2024
@jmuzina jmuzina marked this pull request as ready for review August 16, 2024 19:07
Copy link
Contributor

@pastelcyborg pastelcyborg left a comment

Choose a reason for hiding this comment

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

Looks like the combined demo link doesn't work

@@ -0,0 +1,127 @@
# Params
# title (string) (required): Title of the rich vertical list
# flipped (boolean) (optional): Whether the list items are flipped so image is on the left and the text is on the right. Defaults to false.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably change this to the new variant param as discussed, to allow for more flexibility in the future if new layouts are designed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just for clarification, are you looking for the integer variant approach as explored in #5304 ? If so, I think we should have a wider team discussion on this approach before committing to it

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - still a bit hesitant to merge this until we have a conclusion there, because I suspect we're going to have to change the way we handle variants/variant params, and I'd like to figure that out before we add any new macros to the codebase

Copy link
Member

Choose a reason for hiding this comment

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

I don't want to be blocking this on the decision. We already have couple patterns merged that use flags for variants, so this follows existing convention.

Obviously, it would be good to have a conclusion before we officially publish macros (not to change API later), but I think we are fine doing any changes if needed before that.

templates/_macros/rich-vertical-list.jinja Outdated Show resolved Hide resolved
templates/_macros/rich-vertical-list.jinja Outdated Show resolved Hide resolved
@jmuzina
Copy link
Member Author

jmuzina commented Aug 16, 2024

Looks like the combined demo link doesn't work

Fixed, it was just pointing to the wrong url :)

@lyubomir-popov
Copy link
Contributor

looks great overall! 1 small glitch I noticed: some blurry logos - can you please use the official ones uploaded here https://assets.ubuntu.com/manager?tag=variable-width-logos-july-2024

docs suggestions:

  • flipped: mention that this is useful when having multiple vertical rich lists in a sequence, alternating between flipped and default#

  • this feels too empty. Can we say we should have at least one of the optional elements, e.g. a paragraph or list?

image

@jmuzina
Copy link
Member Author

jmuzina commented Aug 20, 2024

1 small glitch I noticed: some blurry logos - can you please use the official ones uploaded here https://assets.ubuntu.com/manager?tag=variable-width-logos-july-2024

The only Intel logo I found here doesn't look quite right - it has a dotted border and is too wide. Do you have a better one?
Screenshot 2024-08-20 at 4 03 50 PM

Also, the mockup uses Intel logo twice in the same logo section - it doesn't really matter for the example's sake, but I figured I'd mention it in case there is another logo we could use

Your other comments have been addressed!

@lyubomir-popov
Copy link
Contributor

demo not working

@jmuzina
Copy link
Member Author

jmuzina commented Aug 22, 2024

demo not working

@lyubomir-popov I've requested the demo to restart in Jenkins, it should be up in a few minutes if things are working properly

update: demo is now up

@@ -0,0 +1,127 @@
# Params
# title (string) (required): Title of the rich vertical list
# flipped (boolean) (optional): Whether the list items are flipped so image is on the left and the text is on the right. Defaults to false.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - still a bit hesitant to merge this until we have a conclusion there, because I suspect we're going to have to change the way we handle variants/variant params, and I'd like to figure that out before we add any new macros to the codebase

templates/_macros/rich-vertical-list.jinja Outdated Show resolved Hide resolved
@jmuzina
Copy link
Member Author

jmuzina commented Aug 22, 2024

Tomorrow I'll update the images on this to use the new responsive image container. They get quite tall on smaller screens, so we can take advantage of a wider aspect ratio in those cases to make things look better.

@jmuzina jmuzina marked this pull request as draft September 4, 2024 14:01
Namespace rich vertical list with vf_

Rename `numbered` -> `ordered` for rich vertical list

Rename `flipped` param to `is_flipped`

tweak rich list release notes for capitalization consistency
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants