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

Spacing-function for unified spacing dimensions #515

Closed
1 of 2 tasks
flaming-codes opened this issue Apr 12, 2021 · 6 comments · Fixed by #531
Closed
1 of 2 tasks

Spacing-function for unified spacing dimensions #515

flaming-codes opened this issue Apr 12, 2021 · 6 comments · Fixed by #531
Assignees

Comments

@flaming-codes
Copy link

Feature request 🚀

  • I will create Pull Request
  • It's just a suggestion

Expected

To improve consistency across a user's app, I think a well defined Design System should provide a way to set dimensions as a multiple based on a "ground truth" value. An example would be Material UI, where the theme provides a spacing function that returns a number as a multiple based on the input number.

The benefit would be a uniform distancing between components. This would be a "low level" enhancement for the already available theme.layout. ....

Examples

function Component(){
  const theme = useTheme();

  // If we use a base value of '8', this would yield 16px for 'marginTop'.
  return <div style={{ marginTop: theme.spacing(2) }} />
}

Others (Optional)

This is my first issue in this repo and I think it would be nice addition to an already great ui lib. If desired, I can create a PR as POC for it.

@unix
Copy link
Member

unix commented Apr 13, 2021

Good points. I totally agree with you, in fact, have had others complain to me about this in chats.

I think the material is very worth learning from, In our system we can try to implement it like this:

  • Add some public spacing attributes to the theme, used to replace the abused gap: margin / padding.
  • The theme sets the base values for these properties by default: layout: { margin: '10px' }, then we can migrate themargin of most internal components to the Theme.

Users can use it in the following way:

1. Use theme presets directly:

const myMargin = `calc(2 * ${theme.layout.margin})`

return <div style={{ margin: myMargin, padding: theme.layout.padding }} />

2. Users modify the base value by customizing the theme:

const myTheme = Theme.createFromLight({
  type: 'coolTheme',
  layout: {
    margin: '50px'
  },
})

3. Allow users to modify the value of individual components:

<Spacer y={3} margin="1px" />
<Card padding="20px" />

4. As an advanced usage, passing a string is a different meaning from a number:

// Means 2 times the base spacing
// 2 * '10px'
<Card margin={2} />

// Means '2px'
<Card margin="2px" />

The above idea is similar to your proposal. but the implementation is a bit different. I would like to use the fact that when using theme.layout.margin you get a value instead of a function, which would be simpler.

There are a lot of details to think about here and would like to hear your thoughts.

@flaming-codes
Copy link
Author

This looks quite exciting! Looking at your proposal, the following thoughts come to my mind:

Wording

I've noticed you're using both margin and padding as variables for a defined base space. My inital mental model only included a single value, for example theme.layout.unit, which would represent the single base value for all spacing in the theme + the base spacing value for the user to access as well. As I'm new to Geist UI, is there a special difference to distinguish between padding/margin?

Having said that, I really like the idea of themed space values for padding and margin - they as well could be based on a fictional unit-space in this scenario.

String/number prop

This actually looks like a great developer UX! I really appreciate the proposal and think two things should be determined before going all-in on such a change:

  • are there an potential performance penalties? thinking of if/else checks in the component that add (only a very slight) overhead
  • how ergonomic/self-explanatory will it be for new users, i.e. can we expect an intuitional understanding that a number is a multiplier? might be only very minor and work out just fine

Overall, I think your proposal is pretty much spot on regarding a first draft for a new spacing-spec. I'm looking forward on your further feedback! As you've said, the changes would be quite subsential, therefore adequate planing is required.

@unix
Copy link
Member

unix commented Apr 13, 2021

@flaming-codes
Thanks for your thoughts, I summarized on your basis and got a good idea: Maybe we can make all components get scaling capabilities.

About units

Right now Geist's code is making extensive use of a unit called gap, which has the exact value of 16pt, Although users can customize the size of the gap in Theme, they cannot precisely change the margin and padding of the component.

How components work now:

For example, the Card component uses the gap from theme:

// card.tsx
margin: calc(theme.layout.gap * 1.2);
padding: calc(theme.layout.gap * 0.4);

There may even be other spacings that rely on gap, so when the user arbitrarily changes the value of the gap, it may cause the layout to crash.

Thoughts on Proportionality

After thinking for a while, I think this design model should have the following goals:

  1. (Important) The space contained in the component should be scalable, but the font should not be scaled. (FontSize depend on the user's settings, or if necessary, we can consider going in.)
  2. width, height, padding, margin, etc. should follow the theme and should also be able to be set on individual components.
  3. Components should be able to be scaled directly. (with props)

So, for the Card component, it can be designed as follows:

type Props = {
 margin = 1, padding = 0.2, width, height, unit ... // add an extra unit
}

// merge settings, the actual work will use the 'cache function' instead of
const theme = useTheme()
const mergedMargin = margin || theme.layout.margin
const mergedUnit = unit || theme.layout.unit

styles = {
  margin: mergedMargin * mergedUnit;
}

The user has great freedom in using the components:

// Double margin
<Card margin={2} />

// larger padding, smaller margin, scaling by unit
<Card padding={1.5} margin={0.5} />

// Scaling all values
// This will change all the margin values that can be changed at the same time
<Card unit="10pt" />

// Very few scenarios where the user needs to define an exact value
// If it is an exact value, the value will be out of the scaling system
<Card margin="10vw" />

// The margin will no longer follow the unit scaling,  because the margin is defined with an exact value
<Card margin="10vw" unit="20pt" />

I was inspired by your unit idea so that we could get a simpler system for scaling, but one that would work very powerfully. Even any component can be scaled to the size of the frame, and a part of the frame can be precisely defined. In our theme, there is still only one value for unit.

// unit
User settings on components >  User settings on Themes > default Themes > default Component

Ease to use

  1. For each user, in most scenarios they only need to focus on the unit: changing the unit will scale the component. It's easy, and most of the time it's enough.
  2. When the user needs to adjust a property, then scale the property: height={20}. It is also very easy to increase the height to 20 times.
  3. For advanced users, who need to define precise values to meet their design requirements, they can set the property to the exact value to do this.

If everything goes very well, we can even remove all size related props, dynamic scaling each component with unit. I have to say, it's also so cool that! 😃 If we can adjust each component and make them work, this will be an unprecedented library of components.

Performance

The current Geist code already uses a lot of dynamic styles, in fact, the performance is still very high, we just need to add some memo-based tool functions to automatically merge the properties, for a simple value comparison between true and false, this is almost no burden.

As you can see in the Grid component (One of the dynamically styled components), we have cleverly left the rendering to CSS and actually got a Grid component that is far superior to other libraries, with excellent performance.


These are my thoughts just now, welcome to leave your own views. No need to rush to a conclusion, I think this will be a relatively large change. (also very exciting new feature)

@flaming-codes
Copy link
Author

@unix Lit! This sounds very exciting! I agree with you that there's no need to rush things. In the meantime, what open issue do you think would be a good first one for me to start contributing to Geist? Thanks for you awesome work, looking forward to providing some of my coding foo ;)

@unix
Copy link
Member

unix commented May 3, 2021

I'm trying to add this feature in #531, but it's still a first draft. Anyone can leave comments on the details of the implementation.

@unix unix linked a pull request Jun 19, 2021 that will close this issue
8 tasks
@unix unix self-assigned this Jun 19, 2021
@unix
Copy link
Member

unix commented Jun 25, 2021

This feature has been released in version v2.2.0-canary.7.
release nodes

@unix unix closed this as completed Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants