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

fix(label): allow signposts inside labels #937

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

dtsanevmw
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • If applicable, have a visual design approval

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Currently if you put signposts inside labels they are not working due to that <label> be default is spreading a click event to its children and that's causing the signpost to automatically close and not appear.
Example:
If you have a label attached to an input or input inside a label and click the label - the input will get focused.

Issue Number: CDE-9 and #271

What is the new behavior?

Signposts are now working inside <label>.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

👋 @dtsanevmw,

  • 🙏 The Clarity team thanks you for opening a pull request
  • 🎉 The build for this PR has succeeded
  • 🔍 The PR is now ready for review
  • 🍿 In the meantime, view a preview of this PR
  • 🖐 You can always follow up here. If you're a VMware employee, you can also reach us on our internal Clarity Support space

Thank you,

🤖 Clarity Release Bot

@dtsanevmw dtsanevmw force-pushed the dtsanevmw/cde-9 branch 3 times, most recently from 67ec79b to bc86de3 Compare September 7, 2023 03:39
*/
@HostListener('click', ['$event'])
onClick(event) {
if (event.target.hasAttribute('clrSignpostTrigger')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add unit tests to validate both branches here (if and else)?

Copy link
Contributor Author

@dtsanevmw dtsanevmw Sep 7, 2023

Choose a reason for hiding this comment

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

Added in f9c41c0. Thanks!

Copy link
Contributor

@williamernest williamernest Sep 7, 2023

Choose a reason for hiding this comment

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

This doesn't catch all instances and leads to some odd behavior.

  • Clicking inside the signpost when opened will cause it to close. The signpost should be able to receive focus and stay open.
  • The events within the signpost (close button, click in signpost-content) still bubble up to the label and are not defaulted. I would assume that since clicking the icon itself doesn't bubble up and cause the input/control to be selected, the behavior would be the same for clicking within the signpost itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a more strict check to prevent that. Actually the click events were still coming from the label due to the condition not being met. So when you click inside the signpost it's again in the label and label is spreading that to the button inside signpost which is closing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Much better UX now. Thanks for updating.

@@ -8,7 +8,13 @@ <h1>Angular - {{layout}} - {{ labelSize ? 'grid' : 'no grid' }}</h1>

<form clrForm [clrLayout]="layout" [clrLabelSize]="labelSize">
<clr-checkbox-container>
<label>Checkbox</label>
<label
Copy link
Contributor

Choose a reason for hiding this comment

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

Hovering over the signpost or label when the checkbox/radio label causes the close icon to switch to a darker color. This change doesn't apply to the other labels (input, date picker, select, etc). They should be consistent.

Not-hovered:
image

Hovered:
image

@dtsanevmw
Copy link
Contributor Author

Will wait the popover refactor #1435 to be merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants