8000 fix(label): added disabled styles by mcoker · Pull Request #6445 · patternfly/patternfly · GitHub
[go: up one dir, main page]

Skip to content

fix(label): added disabled styles#6445

Merged
srambach merged 2 commits intopatternfly:v6from
mcoker:issue-6442
Mar 25, 2024
Merged

fix(label): added disabled styles#6445
srambach merged 2 commits intopatternfly:v6from
mcoker:issue-6442

Conversation

@mcoker
Copy link
Contributor
@mcoker mcoker commented Mar 19, 2024

fixes #6442

@patternfly-build
Copy link
Collaborator
patternfly-build commented Mar 19, 2024

@mcoker mcoker linked an issue Mar 19, 2024 that may be closed by this pull request
Copy link
Collaborator
@andrew-ronaldson andrew-ronaldson left a comment

Choose a reason for hiding this comment

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

i think it looks good. Thanks

Comment on lines +44 to +49
{{> label
label--IsRemovable=true
label--IsDisabled=true
label-content--IsLink=true
label--id=(concat label--variants--id '-status-link-removable-disabled')
label-text--value=(concat label--variants--title ' link removable (disabled)')}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should show link labels being disabled (at least not the whole label). While you can't click the link to activate it, you can still activate/focus it via keyboard. Also not sure when it may make sense to disable an <a> element, and at that point it's not really a link but rather just text.

If this variant could be updated so that only the "remove" button is disabled, but the rest of the label has its normal styling (and the link works on mouse click) it could be worth keeping in, but otherwise probably better to just remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great feedback! Just for reference from our convo offline, adding for now since we have this as an established (anti-?)pattern already in the library, and will investigate a better approach to this via the spike issue you created. If we can fit this change in the timeline for penta, that would be great!

@mcoker mcoker requested a review from thatblindgeye March 20, 2024 22:02
@mcoker
Copy link
Contributor Author
mcoker commented Mar 20, 2024

@thatblindgeye would you mind re-reviewing when you get a chance? Added aria-hidden and tabindex="-1" (matches the button and other places we're disabling links/non-buttons), which I just missed when making this update initially.

@thatblindgeye
Copy link
Contributor

@mcoker left a comment on the other label PR, but did you mean to use aria-disabled instead to match the button as links example?

Copy link
Member
@srambach srambach left a comment

Choose a reason for hiding this comment

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

👍

@srambach srambach merged commit 71bda59 into patternfly:v6 Mar 25, 2024
@patternfly-build
Copy link
Collaborator

🎉 This PR is included in version 6.0.0-alpha.106 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

Add disabled state for interactive labels

5 participants

0