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

UIP-2762 More convenient ubiquitous access of DOM/aria props #119

Merged

Conversation

greglittlefield-wf
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf commented Oct 5, 2017

Ultimate problem:

I was bored after work yesterday 😄

Adding aria/DOM props to components is a little clunky; you have to do something like this:

(Button()
  ..addProps(domProps()..draggable = true)
  ..addProps(ariaProps()
    ..controls = 'my_popover'
    ..labelledby = 'my_label'
  )
)('Click me')

What would be nicer is a syntax like this:

(Button()
  ..dom.draggable = true
  ..aria.controls = 'my_popover'
  ..aria.labelledby = 'my_label'
)('Click me')

How it was fixed:

  • Add aria and dom getters to UbiquitousDomPropsMixin that provide typed views into a given UiProps class that allow for reading/writing aria/DOM props.
  • Add doNotGenerate option to @Accessor() annotation to allow declaring fields in accessor (props, props mixin, state, state mixin) classes that shouldn't be generated into key-value-pair-backed getters/setters.
    • Necessary to implement caching for aria/dom getters.

Testing suggestions:

  • Verify that tests pass.
  • Render a component with the above syntax and verify that things work as expected.

Potential areas of regression:

This could break consumer code if they've declared props named aria or dom.


FYA: @greglittlefield-wf @aaronlademann-wf @jacehensley-wf @clairesarsam-wf @joelleibow-wf

@aviary-wf
Copy link

Raven

Number of Findings: 0

Copy link
Contributor

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

+1

@@ -243,12 +243,16 @@ class Accessor {
/// The error message displayed when the accessor is not set.
final String requiredErrorMessage;

/// Whether to skip generating an accessor for this field.
final bool doNotGenerate;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a constant annotation for this? @doNotGenerate obviously not needed for this PR but might be nice for later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I thought about adding one but decided against it since this won't be used very often (we haven't had a need for it until now).

@rmconsole3-wf rmconsole3-wf changed the title More convenient ubiquitous access of DOM/aria props UIP-2762 More convenient ubiquitous access of DOM/aria props Oct 12, 2017
@jacehensley-wf
Copy link
Contributor

QA +10

  • Build successful
  • tests pass

  • Testing instruction
  • Dev +1's
  • Unit tests created/updated
  • All unit tests pass
  • Rosie ran/Rosie comment displays expected info
  • Dependency Scan Clean

Merging.

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.

5 participants