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 (do it in your IDE, see if it suggests specific values):
    • 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.6.0

🚀 View preview at
https://instructure.github.io/instructure-ui/pr-preview/pr-1812/

Built to branch gh-pages at 2025-02-12 11:57 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@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

@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 4 times, most recently from 584d176 to 7684341 Compare December 13, 2024 09:53
@balzss balzss requested a review from matyasf December 13, 2024 11:04
@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!

@balzss balzss force-pushed the new-spacing-system branch from 7684341 to 2332fd6 Compare February 5, 2025 21:03
@balzss balzss requested a review from HerrTopi February 5, 2025 21:03
Copy link
Collaborator

@matyasf matyasf left a comment

Choose a reason for hiding this comment

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

Looks good except that the Margin prop stories are not in Storybook, I think you need to add them to stories.ts
Also can you check the Storybook stories and accept them if they look good?

@balzss balzss force-pushed the new-spacing-system branch from 2332fd6 to 581f246 Compare February 11, 2025 10:43
@balzss balzss requested a review from matyasf February 11, 2025 11:32
@balzss balzss force-pushed the new-spacing-system branch from 581f246 to 34424ca Compare February 12, 2025 11:52
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