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

Adjust p-dividers on /pro/why-pro to align with vanilla #13884

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

petesfrench
Copy link
Contributor

@petesfrench petesfrench commented May 24, 2024

Done

  • This change is to align with a fix in vanilla to the p-divider class, I had to update it to fit a specific screen size where the content was overlapping instead (see screenshot below)

QA

Issue / Card

Fixes n/a

Screenshots

before:
image
after:
image

Help

QA steps - Commit guidelines

@webteam-app
Copy link

@jmuzina
Copy link
Member

jmuzina commented May 24, 2024

@petesfrench What browser & dimensions was the "before" snapshot taken in?

I'm able to slightly reproduce this on u.com but not to the severity shown in your "before" screenshot:

Screenshot 2024-05-24 at 9 10 24 AM

@jmuzina
Copy link
Member

jmuzina commented May 24, 2024

@petesfrench Haven't investigated your demo code yet, but my first read from looking at the code as it currently exists on u.com is that you wouldn't need the no-divider class to remove the first divider if the first divider block was the first element in the divider.

Because your <p> with "Available everywhere" is also a child of the divider, it causes the need for no-divider. Maybe you can place it in another element on top of (outside) the divider.

The divider line is being hidden for divider blocks that are the first child of their parent.

See sass src: https://github.com/canonical/vanilla-framework/blob/d0197b27f5733789d6d23536eabe68356e045ae1/scss/_patterns_divider.scss#L47

I'll check out the responsiveness issue now

@jmuzina
Copy link
Member

jmuzina commented May 24, 2024

See comment from @bartaz on avoiding future use of p-divider here.

@petesfrench
Copy link
Contributor Author

@jmuzina I have chosen to include the initial divider here as otherwise it looks like this:
image
I am on Ubuntu/Chrome.

@petesfrench
Copy link
Contributor Author

Hey @jmuzina, sorry, could you take another look at this please? It got a little lost

@lyubomir-popov
Copy link
Contributor

lyubomir-popov commented Jun 25, 2024

To me, this makes the page look a little broken. Why are we using this component on the rebranded pages? It is part of the old style we're moving away from, and as you can see, the line protrudes strangely on the left of things:
image

It also leads to a broken grid:
image

As far as I'm aware, the divider component was meant to be used at the full fixed width, so this would fall under incorrect usage, rather than something that requires an enhancement. Happy to provide an alternative design if UX is ok with me doing so?

@petesfrench
Copy link
Contributor Author

@lyubomir-popov What should we do here? Is there a quick fix we can apply?

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.

4 participants