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

Add width classes #413

Merged
merged 1 commit into from
Jan 9, 2018
Merged

Add width classes #413

merged 1 commit into from
Jan 9, 2018

Conversation

hannalaakso
Copy link
Member

@hannalaakso hannalaakso commented Jan 5, 2018

This PR

  • Adds width classes, corresponding to form-control classes used in Elements but extending their use case
  • Makes them into override classes because:
    • They need to be available to input, select and textarea components while overriding their width styles. Helpers do not override component styles
    • As overrides, they are also available to use cases such as setting width of text
  • Makes naming of numbers in class names consistent with govuk-o-grid
  • Adds examples to /examples/form-alignment (and close an unclosed form tag in the example above)

Trello ticket: https://trello.com/c/jEhFTAqY/458-add-width-form-control-in-elements-classes

Notes:

There is an old comment by Gemma in Elements about updating these form controls but it's not clear what that would have referred to. Just in case it jogs anyone's memory 🙂

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-413 January 5, 2018 13:20 Inactive
@NickColley
Copy link
Contributor

NickColley commented Jan 5, 2018

If these change the widths of the components, is this something that can be done with the other utility/helper classes?

@36degrees
Copy link
Contributor

The form-controls file has not been included as part of the commit.

@hannalaakso hannalaakso force-pushed the add-form-control-classes branch from 8efdd68 to 5c159a1 Compare January 5, 2018 14:36
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-413 January 5, 2018 14:36 Inactive
@hannalaakso
Copy link
Member Author

Fixed now.

@36degrees
Copy link
Contributor

The classes feel like they're a lot more generic than just form controls – but I'm not sure what a good name for them would be.

Also, they need to be wrapped in an @include exports… – at the minute I think you'll find those classes will be appearing many many times in the generated CSS.

@NickColley
Copy link
Contributor

NickColley commented Jan 5, 2018

I have a feeling that these classes might not need to exist if they were wrapped in something that limits line-length, since that seems to be the purpose of these classes.

This would mean the text and the inputs could share the same method of limiting the width without using a grid. (Would allow the use of max-width: x rather than a media query approach also.)

@alex-ju
Copy link
Contributor

alex-ju commented Jan 5, 2018

I remember @dashouse iterated the idea of using ex units for setting the width of an input based on expected number of characters. It's worth discussing with him about it.

@hannalaakso hannalaakso force-pushed the add-form-control-classes branch from 5c159a1 to 87770f6 Compare January 8, 2018 09:41
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-413 January 8, 2018 09:42 Inactive
@hannalaakso
Copy link
Member Author

Thanks @36degrees, I've added a @include exports wrapper.

@hannalaakso hannalaakso force-pushed the add-form-control-classes branch from 87770f6 to 298112f Compare January 8, 2018 12:00
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-413 January 8, 2018 12:01 Inactive
@hannalaakso hannalaakso changed the title Add form-control classes Add width classes Jan 8, 2018
@hannalaakso
Copy link
Member Author

This PR can now be reviewed.

After various conversations, the classes are now width overrides. I've updated the PR description accordingly.

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

👌

@hannalaakso hannalaakso force-pushed the add-form-control-classes branch from 298112f to 6f1607a Compare January 8, 2018 12:28
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-413 January 8, 2018 12:29 Inactive
@dashouse
Copy link

dashouse commented Jan 8, 2018

I think my comment got lost in the ether.

Is it an issue that the grid uses "two-thirds" and the width classes use "2-3"?

@hannalaakso hannalaakso force-pushed the add-form-control-classes branch from 6f1607a to 9820f90 Compare January 8, 2018 17:45
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-413 January 8, 2018 17:45 Inactive
@hannalaakso hannalaakso force-pushed the add-form-control-classes branch from 9820f90 to 6e25474 Compare January 8, 2018 17:46
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-413 January 8, 2018 17:47 Inactive
@hannalaakso
Copy link
Member Author

@dashouse: The naming is now consistent with govuk-o-grid.

@dashouse
Copy link

dashouse commented Jan 9, 2018

Update on naming looks good

Only concern is one-eighth as it will get very small before going to 100%. I'm not sure it's that usable? Don't mind it being in the code but haven't referenced it in the Design System layout page.

These classes correspond to form-control classes used in Elements but extend their use case

Make them into override classes because they need to be available to input, select and textarea components while overriding their width styles. Helpers do not override component styles and as overrides, they are also available to use cases such as setting width of text

Make naming of numbers in classes consistent with `govuk-o-grid`

Add examples to /examples/form-alignment (and close an unclosed form tag in the example above)
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

LGTM!

@hannalaakso
Copy link
Member Author

@dashouse As per our chat just now, I've removed the one-eighth class and created a card to review whether we should make the width classes more customisable for different breakpoints.

@dashouse dashouse assigned dashouse and unassigned dashouse Jan 9, 2018
@dashouse dashouse self-requested a review January 9, 2018 10:45
@hannalaakso hannalaakso merged commit ef5ae47 into master Jan 9, 2018
@hannalaakso hannalaakso deleted the add-form-control-classes branch January 9, 2018 11:39
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.

6 participants