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

feature: spacing utilities #54

Merged
merged 6 commits into from
Nov 20, 2023
Merged

Conversation

marlonmarcello
Copy link
Contributor

@marlonmarcello marlonmarcello commented Nov 18, 2023

Description

We need utility classes. But we don't want them to clash with other libraries and potentially client styles.

Solution

We are using CSS modules to leverage localized class names but exposing them through an object.
Opening this PR to get some feedback on how we feel about this style of utilities.

Usage

import { classnames, spacing } from '@local/utilities'

function Comp() {
  return (
    <div className={classnames([
      spacing.margin[2],
      spacing.lg.margin[4],
      spading.padding[3],
      spacing.xl.margin.x.auto
    ])} />
  )
}

@marlonmarcello marlonmarcello added enhancement New feature or request COMP Tickets related to wethegit-components labels Nov 18, 2023
@marlonmarcello marlonmarcello added this to the v1 milestone Nov 18, 2023
@marlonmarcello marlonmarcello linked an issue Nov 18, 2023 that may be closed by this pull request
@liamegan
Copy link
Member

liamegan commented Nov 18, 2023

I feel like the alternative to the clashing problem is to namespace the classes, right?

The potential cost of this update is that it makes addressing the classes somewhat ambiguous. Do we imagine having to constantly refer to the documentation to figure out the class chain?

Is that assessment fair? Does it miss anything?

@marlonmarcello
Copy link
Contributor Author

marlonmarcello commented Nov 18, 2023

Good points @liamegan
You are right. We can namespace it easily. That is a simple alternative indeed and how I was going to initially go with.

I changed my mind mainly because SCSS is a pain to debug and document dynamic classes like this.
Unless your painstakingly read through the logic that creates theses classes there is no way for you to know the final options you have just by importing this file.

Whereas with a simple typed object, we get this DX:

Screen.Recording.2023-11-17.at.5.16.31.PM.mov

You don't even need the documentation.

That all being said. This does create an extra layer of complexity because the SCSS now needs to be represented in JS as well. But these utilities are NEVER changed. The ones that come with Corgi for example we never ever edit them.

On thing to note is that It would also be easy to simply remove this JS layer if required too and just use the classnames. You'd just import them in the global styles and done!

@rvno
Copy link
Contributor

rvno commented Nov 18, 2023

Asking to confirm - this is the same as Corgi's utils-spacing.scss, but with the example usage referencing the classes via an object instead of manually typing out the classnames like margin-bottom--2 to avoid potential clashing right?

I wouldn't be opposed to it but after seeing the recording I could imagine the autocomplete getting a bit frustrating - but
that's just a personal thing. Would it be possible to fill in the autocomplete tooltips with text displaying the expected spacing as you're typing and sifting through options?

This may also just be a stylistic thing, but seeing spacing.xl.margin[2] would make me think that that the margin[2] value would be different from spacing.md.margin[2] if that makes sense. Probably trivial and a me thing, but I think that because we're in the JS space here the notation makes me initially think of an object below.. but I guess we could also just destructure spacing if needed to have something like const { default, xl } = spacing and xl.margin[2] if we ever have a case where the --base-spacing varies across viewports. (Sorry these thoughts were a bit disorganized).

spacing: {
  md: {
    0: 8px,
    1, 16px,
  },
  ..,
  xl: {
    0: // a different number,
    1: // another different number
  }
}

P.S. song 🎸

@marlonmarcello
Copy link
Contributor Author

marlonmarcello commented Nov 18, 2023

@rvno great notes!

This works the same way as the utils we have right now. The values are 8px based so margin[2] = 8 * 2.
Doesn't matter the breakpoint. The breakpoint is to change the value in THAT breakpoint.

I improved the documentation following your feedback.

Also you brought up a good point maybe I should move the breakpoint key to a different position.
Instead of spacing.md.margin[2] we would have spacing.margin.md[2] or something like that. I believe that's how the CSS classes are actually. What do you think?

@rvno
Copy link
Contributor

rvno commented Nov 18, 2023

@marlonmarcello I'm personally fine with either haha. While the latter matches the classes, I'm now beginning to read the former as as the "2nd option from the margin values", as opposed to the latter where it's the "2nd value from the mediums" 😆 .

@marlonmarcello
Copy link
Contributor Author

@rvno I was reviewing Tailwind docs and they do the same thing where the number represents the value that is multiplied but they do breakpoints with a different suffix, like:

<div class="m-2 lg:m-4"></div>

Don't think we should do that though otherwise we'd have to mix strings with dots.

@marlonmarcello
Copy link
Contributor Author

marlonmarcello commented Nov 18, 2023

@rvno we could the word instead of the number too so we don't use []. Something like:

<div className={classnames([
  spacing.margin.two,
  spacing.lg.margin.four
])} />

@rvno
Copy link
Contributor

rvno commented Nov 20, 2023

Sorry for the late reply @marlonmarcello !

I like how the written out numbers looks but not sure how everyone would feel about writing out the individual numbers so let's see what everyone else thinks - either seem fine to me though.

@marlonmarcello
Copy link
Contributor Author

marlonmarcello commented Nov 20, 2023

Just a reminder that this is an exploration. We can definitely just use these as global utilities like we currently do on Corgi if we want to. It's just a matter of removing the JS and renaming that file.

Copy link
Member

@andrewrubin andrewrubin left a comment

Choose a reason for hiding this comment

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

This is an interesting solution @marlonmarcello, I say let's go for it 👍. Why not take the leap — we can always revert to namespaced utility classes if we decide that works better for us in the future. Nice work!

Just a couple suggestions here:

  • can we add one section to the docs that use the top or left values, just to remove any ambiguity around that usage?
  • minor typos in the docs

Base automatically changed from feature/comp/navigation to release/v2-4-4 November 20, 2023 18:12
@marlonmarcello marlonmarcello merged commit 6b82aa8 into release/v2-4-4 Nov 20, 2023
2 checks passed
@marlonmarcello marlonmarcello deleted the feature/comp/spacing branch November 20, 2023 18:14
@marlonmarcello marlonmarcello mentioned this pull request Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP Tickets related to wethegit-components enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[COMP][styles] margin & padding utilities
4 participants