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

replace {{did-insert}} modifiers in LocaleChooser component #7922

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

stopfstedt
Copy link
Member

@stopfstedt stopfstedt commented Jul 2, 2024

refs ilios/ilios#5374

an alternative solution to #7919

@michaelchadwick
Copy link
Contributor

I nominate your alternative solution after our discussion.

@jrjohnson
Copy link
Member

Oh this is nice, I was originally thinking we'd have to go with @michaelchadwick's solution in a bunch of places where we use did-insert because it's actually hooked up to the DOM so it makes sense. This works very well though, especially the focus on index like that a lot.

sometimes, the given element is all we need.
…at element.

we have full control over this component's markup, so let's traverse it
to markup from the click target to the toggle button,
instead of tracking the toggle button separately.
one less modifier.
@stopfstedt stopfstedt marked this pull request as ready for review July 5, 2024 16:44
@@ -98,3 +98,5 @@ add|ember-template-lint|no-at-ember-render-modifiers|10|6|10|6|d919d2af254f782c0
add|ember-template-lint|no-at-ember-render-modifiers|11|6|11|6|940005188b476a060b0e5d3f05baea24ba178878|1719360000000|1734915600000|1750464000000|app/components/reports/subject/new/session-type.hbs
add|ember-template-lint|no-at-ember-render-modifiers|10|6|10|6|d919d2af254f782c01fe2ba15416673e52e91124|1719360000000|1734915600000|1750464000000|app/components/reports/subject/new/term.hbs
add|ember-template-lint|no-at-ember-render-modifiers|11|6|11|6|940005188b476a060b0e5d3f05baea24ba178878|1719360000000|1734915600000|1750464000000|app/components/reports/subject/new/term.hbs
remove|ember-template-lint|no-at-ember-render-modifiers|16|4|16|4|46d995f60904f6b6f4b1b6b24bb65b9c97441bc1|1719360000000|1734915600000|1750464000000|app/components/locale-chooser.hbs
Copy link
Member

Choose a reason for hiding this comment

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

If you run pnpm run lint:hbs --compact-todo it will clean this up. Otherwise I think it creates a double entry instead of removing the original.

await component.locales[1].click();
assert.strictEqual(component.locales.length, 0);
assert.strictEqual(component.text, 'Español (es)');
assert.ok(component.toggle.hasFocus);
Copy link
Member

Choose a reason for hiding this comment

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

💯

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