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

chore(treeview): checkbox and selectable example id fix #5061

Merged
merged 14 commits into from Sep 2, 2022

Conversation

blfetche
Copy link
Contributor

@blfetche blfetche commented Aug 30, 2022

fixes #5010

@blfetche blfetche changed the title fix(treeview): checkbox and selectable id fix Fixes #5010 fix(treeview): checkbox and selectable id fix Aug 30, 2022
@patternfly-build
Copy link

patternfly-build commented Aug 30, 2022

@nicolethoen nicolethoen linked an issue Aug 30, 2022 that may be closed by this pull request
Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

This looks really good! One way to verify we aren't using duplicate ID's between examples is to run the axe browser extension from the examples page. That's telling me things are looking great, but there are multiple elements with id="toggle-[n]".

One way to fix could be to use the way you've done this here, and have select-toggle-{{tree-view-node--id}} and check-toggle-{{tree-view-node--id}} (or just make one unique, I think that's all that's needed).

But I'm wondering if we could fix the toggle and these text ID issues by adding tree-view--id="tree-view-checkbox-example" and tree-view--id="tree-view-selectable-example" to the parent tree view hbs partials for the checkbox and selectable examples, then where we set tree-view-node--id="[n]" on a node, update that to be tree-view-node--id=(concat tree-view--id '-[n]'). That's something we do in a lot of other examples, and I'm thinking it might work to fix all of these dupe id issues?


Maybe as a follow up task....

To take that a step further, it would be awesome if we had some kind of increment handlebars helper where we could do something like...

my suggestion above - manual, error-prone

{{#> tree-view tree-view--id="my-tree"}}
  {{> tree-view-node tree-view-node--id=(concat tree-view--id '-1')
  {{> tree-view-node tree-view-node--id=(concat tree-view--id '-2')
{{/tree-view}}

auto-magic

{{#> tree-view tree-view--id="my-tree" tree-view--my-counter=0}}
  {{> tree-view-node tree-view-node--id=(concat tree-view--id '-' (inc tree-view--my-counter))
  {{> tree-view-node tree-view-node--id=(concat tree-view--id '-' (inc tree-view--my-counter))
{{/tree-view}}

In that example inc would be the helper. And you wouldn't necessarily have to define tree-view--my-counter=0 - inc could create it if it isn't defined.

Though I'm not sure how that last bit might be wired up. I suspect we'd need:

  1. The ability to create the counter as a variable, which seems like the tricky/iffy part. Maybe something like this https://stackoverflow.com/a/37152268/1123505. But I dunno if there are risks there, the scope of those vars, when they're destroyed, and if we want to open the door to vars in hbs. I know they aren't there for a reason, they may be the path to the dark side 😂
  2. An inc helper, which seems pretty simple - https://stackoverflow.com/a/37743894/1123505

src/patternfly/components/TreeView/tree-view-node-text.hbs Outdated Show resolved Hide resolved
@blfetche
Copy link
Contributor Author

blfetche commented Sep 2, 2022

Rolled back prior changes to HBS files, only updated file required for this change is TreeView.md

@mcoker figured out why the concat helper wasn't working concat syntax was incorrect

@nicolethoen we implemented a change to TreeView.md that uses a top-level parameter "tree-view--id" that is fed into "tree-view-node--id" by way of concatenation with a number

We also applied "newcontext tree-view--id=tree-view--id" to reset the value of tree-view--id for nodes with children.

@nicolethoen
Copy link
Contributor

@nicolethoen we implemented a change to TreeView.md that uses a top-level parameter "tree-view--id" that is fed into "tree-view-node--id" by way of concatenation with a number

Sounds great!

@blfetche blfetche requested a review from mcoker September 2, 2022 14:28
Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

Love it! Great job @blfetche 💪

@mcoker mcoker changed the title Fixes #5010 fix(treeview): checkbox and selectable id fix chore(treeview): checkbox and selectable example id fix Sep 2, 2022
@mcoker mcoker merged commit 4bceb86 into patternfly:main Sep 2, 2022
@patternfly-build
Copy link

🎉 This PR is included in version 4.212.1 🎉

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug - Tree view: IDs are reused between examples
4 participants