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

Allowed passing @tagName for dynamic tag #46

Merged
merged 5 commits into from
Jul 10, 2020
Merged

Allowed passing @tagName for dynamic tag #46

merged 5 commits into from
Jul 10, 2020

Conversation

ijlee2
Copy link
Owner

@ijlee2 ijlee2 commented Jul 3, 2020

TODO

  • Install ember-element-helper
  • Decide how to allow passing @tagName (e.g. is the value immutable?)
  • Look for examples in demo app that can use @tagName to simplify layout
  • Write rendering tests for @tagName
  • Document the use of @tagName
  • Review RFCs #389 and #620, as well as some dependents to check for unintended side effects

Description

This PR addresses and can close the issue #38.

Based on @chadian's suggestion, I pulled in ember-element-helper as a dependency so that developers can create a dynamic tag (instead of <div>) to facilitate accessibility and semantic HTML.

If I understood correctly the migration plan that ember-element-helper offered in its README:

When this feature is implemented in Ember.js, we will release a 1.0 version of this addon as a true polyfill for the feature, allowing the feature to be used on older Ember.js versions and be completely inert on newer versions where the official implementation is available.

then I believe ember-element-helper will remain a dependency until Ember 3.16 (this addon's current minimum compatibility version) is no longer an LTS (long-term support) version.

Notes

Currently, the ember-element-helper addon allows the tag name to change at a later time. In the original RFC, the tag name was considered to be immutable.

I couldn't think of a use case for mutable tag. To be safe, I set this.tagName in the constructor so that the value stays the same even if @tagName changes.

In the demo app, I tested what would happen if I let <Widgets::Widget-2> update @tagName. I created a tracked property in the component, then updated the value using later() from @ember/runloop. I noticed the rerender of the content inside <ContainerQuery> when the component's tag is updated.

mutable-tag

References

@ijlee2 ijlee2 added the enhance: code Issue asks for new feature or refactor label Jul 3, 2020
@ijlee2 ijlee2 linked an issue Jul 3, 2020 that may be closed by this pull request
@ijlee2 ijlee2 force-pushed the add-dynamic-tag branch 6 times, most recently from bef6ec8 to 1f0c9ff Compare July 9, 2020 04:08
@ijlee2 ijlee2 force-pushed the add-dynamic-tag branch from 1f0c9ff to a08b2b7 Compare July 9, 2020 12:12
@@ -23,6 +23,12 @@ module.exports = function(environment) {
}
};

ENV['ember-a11y-testing'] = {
componentOptions: {
turnAuditOff: true
Copy link
Owner Author

Choose a reason for hiding this comment

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

I turned off ember-a11y-testing in development because I ran into an error while testing window resize:

element-resize-detector.js:165 Uncaught Axe is already running. Use `await axe.run()` to wait for the previous run to finish before starting a new run.

super(...arguments);

// The dynamic tag is restricted to be immutable
this.tagName = this.args.tagName || 'div';
Copy link
Owner Author

Choose a reason for hiding this comment

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

I used the OR operator || so that, in addition to null and undefined (which can be covered by the nullish coalescing operator ??), the empty string '' would also result in the default <div> tag.

Copy link

Choose a reason for hiding this comment

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

Your note on setting this in the constructor and not letting it be dynamic I think is pretty sensible 👍

@ijlee2 ijlee2 marked this pull request as ready for review July 9, 2020 12:29


test('The component continues to have the <div> tag when it is resized', async function(assert) {
await resizeContainer(500, 300);
Copy link

Choose a reason for hiding this comment

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

😍 First time seeing these, very nice. Test helpers were on my list but didn't get a chance to making these properly.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, it's amazing how we can write an ordinary function as a test helper. Writing tests in Ember is quite fun.

@ijlee2 ijlee2 merged commit 85820b6 into master Jul 10, 2020
@ijlee2 ijlee2 deleted the add-dynamic-tag branch July 10, 2020 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhance: code Issue asks for new feature or refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic Root Elements
2 participants