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 spacing and size of logo section and deprecate the dense variant #5252

Merged

Conversation

pastelcyborg
Copy link
Contributor

@pastelcyborg pastelcyborg commented Jul 25, 2024

Done

  • Simplify dense logo section component, removing unnecessary styles and unifying sizes under new 4rem small and 6.5rem large sizes

Fixes #5157

QA

  • Open demo
  • Witness logos being 6.5rem in height
  • Witness margin/padding on logos and logo section also having increased to match new sizings

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.

@webteam-app
Copy link

@pastelcyborg
Copy link
Contributor Author

@lyubomir-popov So there isn't actually a 6.5rem value in our Sass variables; I used medium instead, which is 6rem. It's worth noting that the size of these is defined by their heights, not widths. This is a pretty big size increase; please see demo to confirm.

@pastelcyborg pastelcyborg changed the title Increase size of dense logo section Increase size of dense Logo Section logos Jul 25, 2024
@pastelcyborg pastelcyborg marked this pull request as ready for review July 25, 2024 21:16
@lyubomir-popov
Copy link
Contributor

I would hardcode 6.5rewm. This is not a value we would be reusing anywhere outside, so no need for a variable.

@pastelcyborg pastelcyborg force-pushed the 5157-dense-logo-section-sizes branch from c1e3b16 to 719a789 Compare July 26, 2024 17:04
@pastelcyborg
Copy link
Contributor Author

@lyubomir-popov Done, but it's worth noting that using a value outside the small/medium/large range impacts the use of spacing around the component as well. Let me know if you're okay with this.

@bartaz
Copy link
Member

bartaz commented Jul 29, 2024

@lyubomir-popov Using a new arbitrary value of 6.5rem disturbs a bit the spacing of logo section.

We already have 3 different sizes defined for regular variant on 3 breakpoints, with medium being 6rem. So why introduce another that is just 0.6rem bigger?

Also, we have this "offset" built in that pulls the logos closed together vertically, which is always by 1/8th of the size. Like 8rem desktop: 1rem offset, 6rem medium 0.75rem offset.

The value of 6.5rem does not work well with this math. If we use 6.5/8 we get into small fractions that will throw us off the baseline grid.

@lyubomir-popov
Copy link
Contributor

We already have 3 different sizes defined for regular variant on 3 breakpoints, with medium being 6rem. So why introduce another that is just 0.6rem bigger?

  • I think we should make 6.5rem the default and deprecate the dense version, as you're absolutely right.
  • for the negative margin, can we just say -1rem and leave it at that? there's no need for compliated maths there

@bartaz
Copy link
Member

bartaz commented Jul 29, 2024

Current implementation (not looking at dense variant):

  • large screens: 8rem logos, -1rem margins
  • medium screens: 6rem logos, -0.75rem margins
  • small screens: 4rem logos, -0.5rem margins

@lyubomir-popov what exactly do you suggest changing to 6.5rem value? logo size on desktop? what about others?

@lyubomir-popov
Copy link
Contributor

6.5rem/1rem on medium and desktop, small remains as is

@bartaz bartaz changed the title Increase size of dense Logo Section logos Adjust spacing and size of logo section and deprecate the dense variant Aug 1, 2024
@pastelcyborg pastelcyborg added Review: Percy needed This PR needs a review of Percy for visual regressions and removed Review: Percy +1 labels Aug 1, 2024
@jmuzina
Copy link
Member

jmuzina commented Aug 1, 2024

Jinx! 😂
image

@bartaz bartaz added Review: Percy +1 and removed Review: Percy needed This PR needs a review of Percy for visual regressions labels Aug 1, 2024
bartaz
bartaz previously approved these changes Aug 1, 2024
Copy link
Member

@bartaz bartaz left a comment

Choose a reason for hiding this comment

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

Small suggestion, but lookin good!

releases.yml Outdated Show resolved Hide resolved
jmuzina
jmuzina previously approved these changes Aug 1, 2024
@pastelcyborg pastelcyborg dismissed stale reviews from jmuzina and bartaz via c7767bf August 1, 2024 14:55
@pastelcyborg pastelcyborg merged commit fdc5961 into canonical:main Aug 1, 2024
7 checks passed
@pastelcyborg pastelcyborg deleted the 5157-dense-logo-section-sizes branch August 1, 2024 15:10
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.

p-logo-section--dense: can we make the logo sizes 6.5rem (104px)
5 participants