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

Added "width" attribute to inputs #48

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Added "width" attribute to inputs #48

merged 1 commit into from
Nov 16, 2023

Conversation

apfohl
Copy link
Member

@apfohl apfohl commented Nov 9, 2023

https://community.openproject.org/wp/50740

This PR adds a new width attribute to inputs.

There are five possible values:

  • xlarge: 96rem
  • large: 72rem
  • medium: 48rem
  • narrow: 32rem
  • small: 16rem

I 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:

  • TextField
  • Select

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:

class SingleTextFieldForm < ApplicationForm
  form do |my_form|
    my_form.text_field(
      name: :ultimate_answer,
      label: "Ultimate answer",
      required: true,
      caption: "The answer to life, the universe, and everything",
      width: :narrow
    )
  end
end

@apfohl apfohl added the enhancement New feature or request label Nov 9, 2023
Copy link

changeset-bot bot commented Nov 9, 2023

🦋 Changeset detected

Latest commit: 102a2ee

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

This PR includes changesets to release 1 package
Name Type
@openproject/primer-view-components 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

Copy link
Collaborator

@HDinger HDinger left a 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 no full. In that. As you said, it may be worth to investigate whether we want to set the resulting classes on the same element as the full_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, ...]

@apfohl apfohl force-pushed the input-width-attribute branch 2 times, most recently from 979aa77 to 22e8d87 Compare November 10, 2023 13:50
Copy link

github-actions bot commented Nov 10, 2023

⚠️ Visual differences found

Our visual comparison tests found UI differences. Please review the differences by viewing the files changed tab to ensure that the changes were intentional.

Review visual differences

@apfohl apfohl force-pushed the input-width-attribute branch 3 times, most recently from b22b21e to 78cdd4c Compare November 13, 2023 10:09
Copy link
Collaborator

@HDinger HDinger left a 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.

lib/primer/forms/dsl/input.rb Show resolved Hide resolved
@apfohl apfohl force-pushed the input-width-attribute branch 9 times, most recently from 637ea4b to 59db21d Compare November 14, 2023 14:51
Copy link
Collaborator

@HDinger HDinger 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 to me 👍 Can we remove the changes to the Gemfile.lock? Other than that, it is good to merge from my side.

@apfohl apfohl force-pushed the input-width-attribute branch from 59db21d to 102a2ee Compare November 16, 2023 12:55
@apfohl apfohl merged commit 58bcf23 into main Nov 16, 2023
26 checks passed
@apfohl apfohl deleted the input-width-attribute branch November 16, 2023 13:36
@openprojectci openprojectci mentioned this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants