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

Feat hide clear button #174

Merged
merged 15 commits into from
Sep 10, 2024
Merged

Feat hide clear button #174

merged 15 commits into from
Sep 10, 2024

Conversation

dombesz
Copy link

@dombesz dombesz commented Sep 4, 2024

What are you trying to accomplish?

Introduce a functionality to show and hide the clear button of the SubHeader component filter input. The clear button is shown when the filter input has some value typed in, and it is hidden when the input is empty.

Integration

No required, when the show_clear_button: true argument is passed to the component.with_filter_input, the added functionality will work automatically.

List the issues that this change affects.

https://community.openproject.org/projects/openproject/work_packages/57777/activity

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.
  • Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

Added a new method toggleFilterInputClearButton to the SubHeaderElement which will decide whether the clear button should be displayed or not. This method is then triggered on the filter input focus and input events, ensuring that the clear button is shown only when there is content inside the input field.

Anything you want to highlight for special attention from reviewers?

Would be nice to have the PrimerTextFieldElement#clearContents method updated to emit an input event. That way the toggleFilterInputClearButton shouldn't be bound to both the focus and the input event, just the latter. This has been an issue for another functionality before, so I might need to make that change PR against the main primer repo, not this one.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Lookbook)
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Copy link

changeset-bot bot commented Sep 4, 2024

🦋 Changeset detected

Latest commit: 7b3ed15

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

@dombesz dombesz force-pushed the feat-hide-clear-button branch 9 times, most recently from 0212728 to 45a761f Compare September 5, 2024 14:19
@dombesz dombesz marked this pull request as ready for review September 6, 2024 07:37
@dombesz dombesz requested a review from HDinger September 6, 2024 08:04
@dombesz dombesz force-pushed the feat-hide-clear-button branch 2 times, most recently from eede680 to 64cc54f Compare September 6, 2024 10:33
@dombesz dombesz force-pushed the feat-hide-clear-button branch from 64cc54f to a33d92f Compare September 6, 2024 13:39
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.

I have some minor remarks. I think we should set show_clear_button to true as a default for text inputs in the SubHeaders. Wdyt?

app/components/primer/open_project/sub_header.rb Outdated Show resolved Hide resolved
app/components/primer/open_project/sub_header_element.ts Outdated Show resolved Hide resolved
app/components/primer/open_project/sub_header_element.ts Outdated Show resolved Hide resolved
app/components/primer/open_project/sub_header_element.ts Outdated Show resolved Hide resolved
@dombesz dombesz requested a review from HDinger September 9, 2024 08:56
@dombesz dombesz force-pushed the feat-hide-clear-button branch from 585fce7 to 06ca3d9 Compare September 9, 2024 12:07
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.

Good to merge once the CI is green

@HDinger HDinger merged commit 95adbb8 into main Sep 10, 2024
36 checks passed
@HDinger HDinger deleted the feat-hide-clear-button branch September 10, 2024 07:31
@openprojectci openprojectci mentioned this pull request Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants