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

DS-1933: Removes margins from form web components #9

Open
wants to merge 2 commits into
base: DS-1932/vertical-form-element-spacing
Choose a base branch
from

Conversation

MeaghanCarrieres
Copy link
Collaborator

  • Removes margins from "ontario-input", "ontario-checkboxes", "ontario-button" and "ontario-textarea" web components
  • Removes margins from ontario-input class in text-input component styles in global styles package to target "ontario-date-input" margins

@matt-stjean
Copy link
Member

@MeaghanCarrieres Does anything need to be done for ontario-dropdown-list and ontario-radio-buttons? I see them in the ticket but not the changes.

@MeaghanCarrieres
Copy link
Collaborator Author

@MeaghanCarrieres Does anything need to be done for ontario-dropdown-list and ontario-radio-buttons? I see them in the ticket but not the changes.

Hey @matt-stjean! The ontario-dropdown-list component makes use of the ontario-input class, so when the margins were removed from that, it fixed it.

As for the radio buttons, they don't seem to have any margins on their group, and only on individual radio button items themselves. The margin that is causing the space at the bottom of their grouping is from the ontario-form-group class, which I believe will be removed/refactored in another ticket in this epic.

Copy link
Member

@matt-stjean matt-stjean 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 the reply @MeaghanCarrieres, it looks good to me.

@smorrisods
Copy link
Collaborator

smorrisods commented Dec 16, 2024

Looks like this is what is adding the extra spacing. I think this rule will not be needed with the overall changes to how we handle form spacing as the vertical spacing. I think we should clean this up in this PR as it is relevant to the thematic changes being made.

From: packages/ontario-design-system-global-styles/src/styles/scss/6-components/_text-inputs.component.scss

.ontario-form-group:last-of-type {
	margin-bottom: 2.5rem;
}

It seems to also show up in packages/ontario-design-system-global-styles/src/styles/scss/6-components/_form.component.scss:

.ontario-form-group:last-of-type {
	margin-bottom: 2.5rem;
}

.ontario-form-group:last-of-type {
	margin-bottom: spacing.$spacing-8;
}

We should figure out what to do with the ontario-form-group class, as it impacts the distribution package. Any thoughts?

@MeaghanCarrieres
Copy link
Collaborator Author

Looks like this is what is adding the extra spacing. I think this rule will not be needed with the overall changes to how we handle form spacing as the vertical spacing. I think we should clean this up in this PR as it is relevant to the thematic changes being made.

From: packages/ontario-design-system-global-styles/src/styles/scss/6-components/_text-inputs.component.scss

.ontario-form-group:last-of-type {
	margin-bottom: 2.5rem;
}

It seems to also show up in packages/ontario-design-system-global-styles/src/styles/scss/6-components/_form.component.scss:

.ontario-form-group:last-of-type {
	margin-bottom: 2.5rem;
}

.ontario-form-group:last-of-type {
	margin-bottom: spacing.$spacing-8;
}

We should figure out what to do with the ontario-form-group class, as it impacts the distribution package. Any thoughts?

Hey Scott,
I removed the styles for now, but now that I'm investigating its use, I'm not sure if we need it at all. I originally thought it was used just to contain the labels and inputs, but it seems like those containers are already there for other classes (such as ones to indicate errors or hint expanders, etc.). So, it seems like we can just get rid of it alltogether?

@smorrisods smorrisods added the component-library Component Library package impacted label Dec 16, 2024
@smorrisods
Copy link
Collaborator

smorrisods commented Dec 16, 2024

So, it seems like we can just get rid of it alltogether?

This was the sense I got too a while back, but it needs more detailed exploration to make sure we're not causing carnage by removing it. 🐱

@smorrisods
Copy link
Collaborator

I wonder how this ontario-form-group SASS translates into the dist package 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-library Component Library package impacted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants