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

Add support for ariaRole property #51

Merged

Conversation

jelhan
Copy link
Contributor

@jelhan jelhan commented Jan 16, 2020

This adds support for ariaRole property:

  • If it's value is a static string it's converted to a tagless component.
    export default Component.extend({
      ariaRole: 'button',
    });
    is converted to
    <div role="button"></div>
  • If it's value is dynamic (most likely a computed property) the component is skipped. I expect this to be a rare edge case. So not doing the additional work to do it, might be okay.

This is kind of conflicting with #44. If support for native classes lands before, support for ariaRole in native classes needs to be added here. If this is merged before #44, support for ariaRole has to be added there. I don't care about the order.

Closes #49

lib/__tests__/__snapshots__/transform.js.snap Outdated Show resolved Hide resolved
lib/transform.js Outdated Show resolved Hide resolved
@jelhan jelhan changed the title adds support for ariaRole property WIP: adds support for ariaRole property Jan 16, 2020
@jelhan jelhan force-pushed the issue-49-support-for-aria-role branch 2 times, most recently from ef450ec to 5462648 Compare January 16, 2020 22:41
@jelhan
Copy link
Contributor Author

jelhan commented Jan 16, 2020

Sorry, too late, too tired. HTML attribute should not be applied to the created element and the ariaRole property be removed from the object.

@simonihmig Vielen Dank für den schnellen Review und das wachsame Auge!

@jelhan jelhan changed the title WIP: adds support for ariaRole property adds support for ariaRole property Jan 16, 2020
Copy link
Contributor

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@Turbo87 Turbo87 force-pushed the issue-49-support-for-aria-role branch from 5462648 to 58e9e82 Compare January 20, 2020 21:51
@Turbo87 Turbo87 changed the title adds support for ariaRole property Add support for ariaRole property Jan 20, 2020
@Turbo87 Turbo87 added the enhancement New feature or request label Jan 20, 2020
Copy link
Contributor

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

thanks for implementing this @jelhan! I've rebased this on top of #43 and will merge once it's green :)

@Turbo87 Turbo87 merged commit 7c84f9e into ember-codemods:master Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for ariaRole property of the JS class
3 participants