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

new spacing tokens + spacing docs + margin props for input components #1812

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

Conversation

balzss
Copy link
Contributor

@balzss balzss commented Dec 6, 2024

INSTUI-4387
INSTUI-4367

test plan:

  • read new spacing docs, check for errors and missing info
  • try the new space tokens on some components (like buttons or view)
  • try the new margin prop in the following components:
    • DateInput2
    • TextInput
    • NumberInput
    • ColorPicker
    • FormField
    • TextArea

@balzss balzss self-assigned this Dec 6, 2024
Copy link

github-actions bot commented Dec 6, 2024

PR Preview Action v1.4.8
🚀 Deployed preview to https://instructure.github.io/instructure-ui/pr-preview/pr-1812/
on branch gh-pages at 2024-12-13 11:03 UTC

@balzss balzss force-pushed the new-spacing-system branch 3 times, most recently from 3ea6398 to 82f42a7 Compare December 9, 2024 14:32
@balzss balzss changed the title WIP: new spacing new spacing tokens + spacing docs + margin props for input components Dec 9, 2024
@balzss balzss force-pushed the new-spacing-system branch from 82f42a7 to ac2ee2d Compare December 9, 2024 14:41
@balzss balzss requested review from HerrTopi and matyasf December 9, 2024 14:43
@balzss balzss marked this pull request as ready for review December 9, 2024 14:44
@balzss
Copy link
Contributor Author

balzss commented Dec 9, 2024

@matyasf @HerrTopi this is the first part of the spacing update. this pr adds the tokens, the guide and the margin prop for some components. in order to keep the pr normal size and make the review easier, other components will get the margin prop in other PRs

@@ -225,6 +225,11 @@ type ColorPickerOwnProps = {
* If true, alpha slider will be rendered. Defaults to false
*/
withAlpha?: boolean

/**
* Margin around the component. Accepts a `Spacing` token. See token values and example usage in [this guide](/#layout-spacing).
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should put the full link here, so it works in IDEs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that but then it'll link to the deployed page even when developing, I think it's better practice to only use full urls when it navigates outside the page

Copy link
Collaborator

Choose a reason for hiding this comment

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

Node uses the full links too, e.g.
image

But on the other hand older versions of InstUI should reference older version of our docs..

@matyasf
Copy link
Collaborator

matyasf commented Dec 11, 2024

Nice work, I like it so far :)

It would be good to have a visual test for all these margins. I suggest to make a large single test (like withTooltipPositioning.tsx) that has all the components with a set margin besides each other

@balzss balzss force-pushed the new-spacing-system branch 3 times, most recently from ade04cc to 584d176 Compare December 13, 2024 09:52
@balzss balzss force-pushed the new-spacing-system branch from 584d176 to 7684341 Compare December 13, 2024 09:53
@balzss balzss requested a review from matyasf December 13, 2024 11:04
)
}

export default SpacingTokens
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd also put here all components with some fixed spacing value to test that it works for all of them. something like:

<Button margin="space4">Button</Button>
<NumberInput margin="space4">NumberInput</NumberInput>
<TextInput margin="space4">TextInput</TextInput>
...

@matyasf matyasf self-requested a review December 13, 2024 12:40
Copy link
Contributor

@HerrTopi HerrTopi left a comment

Choose a reason for hiding this comment

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

Lgtm when the already mentioned issues are fixed.

Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants