-
Notifications
You must be signed in to change notification settings - Fork 1
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
Added "width" attribute to inputs #48
Conversation
🦋 Changeset detectedLatest commit: 102a2ee The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
21bff2a
to
295787a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @apfohl
Looks good 👍 As discussed, just some minor remarks:
- Please rename the width names. My suggestion would be to stick to the names Primer uses at other places:
narrow
,small
,medium
,large
,xlarge
and nofull
. In that. As you said, it may be worth to investigate whether we want to set the resulting classes on the same element as thefull_width
parameter. - Please remove the changes to the form examples to minimize potential merge conflicts. Instead you can create a new file and show the examples there.
- In the playground preview of the component, we should add a
width
paramter to document it. `# @param width [Symbol] select [narrow, small, medium, ...]
979aa77
to
22e8d87
Compare
|
b22b21e
to
78cdd4c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The failing test revealed an interesting fact: the autocomplete component already has a width
attribute with the options auto, small, medium, large, xlarge, xxlarge
. It might make sense to take those over. Unfortunately however, they use that attribute not for the input itself but for the dropdown.. I don't quite know how we want to deal with that. If you want to, we can discuss that later.
637ea4b
to
59db21d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍 Can we remove the changes to the Gemfile.lock
? Other than that, it is good to merge from my side.
- TextField - Select
59db21d
to
102a2ee
Compare
https://community.openproject.org/wp/50740
This PR adds a new width attribute to inputs.
There are five possible values:
xlarge
: 96remlarge
: 72remmedium
: 48remnarrow
: 32remsmall
: 16remI chose these value arbitrarily with some looks at our specified designs. So feel free to suggest others or wait for input from design. We can change them at a later point.
The new attribute is active for:
As I only need these right now. But it super easy to add them to other components as well.
You can use the attribute like this: