-
Notifications
You must be signed in to change notification settings - Fork 55
fix(dialogBehavior): move id generation to component #1449
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1449 +/- ##
==========================================
- Coverage 73.67% 73.63% -0.04%
==========================================
Files 807 808 +1
Lines 6089 6092 +3
Branches 1772 1774 +2
==========================================
Hits 4486 4486
- Misses 1598 1601 +3
Partials 5 5
Continue to review full report at Codecov.
|
…b.com/stardust-ui/react into fix/omit-id-generation
} | ||
|
||
static getDerivedStateFromProps(props, state) { | ||
const { autoControlledProps } = state | ||
const { autoControlledProps, getAutoControlledStateFromProps } = state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The single downside that I can't access actual getAutoControlledStateFromProps()
by referencing this
because it's static. Has anyone better idea than keep it in the state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got confused, where this method comes from? Why are we extracting it form the state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me check this part again: https://codesandbox.io/s/elastic-framework-w3kfk
I expect that this should work properly...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React calls it without this
, that's why it's not working:
https://github.com/facebook/react/blob/2b93d686e359c7afa299e2ec5cf63160a32a1155/packages/react-test-renderer/src/ReactShallowRenderer.js#L560
@@ -15,8 +14,6 @@ import * as _ from 'lodash' | |||
* Adds attribute 'aria-labelledby' based on the property 'aria-labelledby' to 'popup' component's part. | |||
* Adds attribute 'aria-describedby' based on the property 'aria-describedby' to 'popup' component's part. | |||
* Adds attribute 'role=dialog' to 'popup' component's part. | |||
* Generates unique ID and adds it as attribute 'id' to the 'header' component's part if it has not been provided by the user. | |||
* Generates unique ID and adds it as attribute 'id' to the 'content' component's part if it has not been provided by the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely should be replaced. @silviuavram @sophieH29 can you help me with rules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can refactor the Dialog-test.tsx
to check that IDs are generated there if they are not provided. I think you can remove the other tests (label, labelledby etc) since they should be checked in the behavior.
In dialogBehavior-test.tsx
you can add individual tests for the generation. Pass header
as a shorthand with id
and check it's added as aria-labelledby
. Similar to content
. The other case when they are applied directly from the aria-labelledby
/ aria-describedby
should already be tested by a rule.
After looking at this file at 'aria-labelledby': defaultAriaLabelledBy || props['aria-labelledby'],
probably you can remove the || props['aria-labelledby']
and do the whole logic in the method, which you can rename without Default
: getAriaDescribedBy
or something. Where you return it as the prop value, if passed, as header['id] if header exists, or nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think @sophieH29 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated UTs 🔍
...state, | ||
...initialAutoControlledState, | ||
autoControlledProps, | ||
getAutoControlledStateFromProps, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a method? Should we invoke it to get the object containing props?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an initial state 🤔 We store it in state and will be able to access it inside of static
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's describe from user perspective how these methods should be defined and used in the PR description. I am a bit confused as why we are introducing this method, and how it should be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update PR description
…b.com/stardust-ui/react into fix/omit-id-generation # Conflicts: # packages/react/src/lib/accessibility/Behaviors/Dialog/dialogBehavior.ts
…b.com/stardust-ui/react into fix/omit-id-generation # Conflicts: # packages/react/src/lib/AutoControlledComponent.tsx # packages/react/src/lib/accessibility/Behaviors/Dialog/dialogBehavior.ts
@@ -142,7 +144,21 @@ class Dialog extends AutoControlledComponent<WithAsProp<DialogProps>, DialogStat | |||
triggerRef = React.createRef<HTMLElement>() | |||
|
|||
getInitialAutoControlledState(): DialogState { | |||
return { open: false } | |||
return { | |||
contentId: _.uniqueId('dialog-content-'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we store this strings to some statics, or reuse the classnames if they exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't there a big benefit because we don't want to expose them to clients
@@ -88,6 +88,8 @@ export interface DialogProps | |||
} | |||
|
|||
export interface DialogState { | |||
contentId?: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use these as an id of the elements, or they are used only for the aria-* attributes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to keep responsibility for this on accessibility behaviors: https://github.com/stardust-ui/react/pull/1449/files#diff-7d8c4d117a2402a4e3fbb997e3c6693fL32
return { | ||
contentId: (props.content && props.content['id']) || state.contentId, | ||
headerId: (props.header && props.header['id']) || state.headerId, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the changes. It would be great if we can extract this logic, as I think it may be reused in other components as well, of course not as part of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we should inspect other cases with leaked React abstractions
content?: { | ||
id?: string | ||
} | ||
header?: string | object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are header
and content
props used anywhere in the behavior's logic? I mean, even from semantical point of view as a client I wouldn't expect them to be necessary to calculate accessibility attributes - am I missing something?
state: DialogState, | ||
): Partial<DialogState> { | ||
return { | ||
contentId: (props.content && props.content['id']) || state.contentId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
props.content && props.content['id']
what if React element is passed to any of these slots, with id
provided? As I see, in that case we will silently override client's ID value - this is definitely what client expects in this situation.
As we've committed to handle the props calculation for React elements provided to slots, we should handle this case as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, will rework it
@@ -23,6 +23,26 @@ import Header from '../Header/Header' | |||
import Portal from '../Portal/Portal' | |||
import Flex from '../Flex/Flex' | |||
|
|||
const getOrGenerateIdFromShorthand = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 probably, in future we will need to move this function to lib
, as the logic of peek if client had specified some prop for shorthand value
seems to be a quite common case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may just suggest to cover React element's case for shorthand slot in tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was done in e816e3c
Problem 1
IDs in
dialogBehavior
are generated on each render. If we will try to have some state there to persist there IDs it will break the idea that all accessibility behaviors are pure functions.Problem 2
Abstraction with slots leaked to components because we accessed there
header['id']
andcontent['id']
.Solution
Move IDs generation to component and keep them in state, IDs will be stored for each slot separately (
headerId
,contentId
) so Stardust slots will not leak.