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

fix: add xs margin top to the top of input #16

Merged
merged 11 commits into from
Sep 1, 2023
Merged

Conversation

kaydbwithers
Copy link
Contributor

@kaydbwithers kaydbwithers commented Aug 30, 2023

Why

This PR adds margin top to the top of the button for DateRangePicker and top of the input for DateInput.

This change makes the spacing consistent with labels from other components.

What

DateRangePicker Before (Notice the spacing below "Date range", compared to "Select report")
image

DateRangePicker After
image

DateInput Before
image

DateInput After
image

@kaydbwithers kaydbwithers requested a review from a team as a code owner August 30, 2023 23:16
@changeset-bot
Copy link

changeset-bot bot commented Aug 30, 2023

🦋 Changeset detected

Latest commit: 0581f7f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@kaizen/date-picker Minor
@kaizen/draft-form Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Aug 30, 2023

✨ Here is your branch preview! ✨

Last updated for commit 0581f7f: fix: rename class

Comment on lines 5 to 8

input {
margin-top: $spacing-xs;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

As opposed to selecting input, create a new class and attach it to the Input instead.

I dived in a little more and realised TextField (I thought it could be used in DateInput, but it seems the icon prevents it from doing so, so sadly can't be done) also has the same issue.
Could you possibly fix the spacing there too? 🙏🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @HeartSquared, that has been updated now.

Copy link
Contributor

@HeartSquared HeartSquared left a comment

Choose a reason for hiding this comment

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

Thanks for adding the extra change! Just realised the usage of the deprecated spacing token (my bad, not your fault), so I've left comments to outline the changes to make :)

@kaydwithers
Copy link
Contributor

@HeartSquared no problem, that's been updated now.

Copy link
Contributor

@HeartSquared HeartSquared left a comment

Choose a reason for hiding this comment

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

Thanks for this! :)

@HeartSquared HeartSquared merged commit a8c5121 into main Sep 1, 2023
21 checks passed
@HeartSquared HeartSquared deleted the kds-1699-margin-top branch September 1, 2023 04:53
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.

4 participants