feat(react-headless-components-preview): expose missing state data attributes - #36480
feat(react-headless-components-preview): expose missing state data attributes#36480Victor Genaev (mainframev) wants to merge 2 commits into
Conversation
📊 Bundle size report
🤖 This report was generated against c83bfa7666778640350f54991c8ebe712a8f389b |
|
Pull request demo site: URL |
Martin Hochel (Hotell)
left a comment
Martin Hochel (Hotell)
left a comment
There was a problem hiding this comment.
left some comments, those +2kb are not great :D
| }); | ||
|
|
||
| state.popover.open = popoverOpen; | ||
| state.root['data-open'] = stringifyDataAttribute(popoverOpen); |
There was a problem hiding this comment.
isn't this boolean ? AFAIR we are going to toggle bool attributes instead of toggling value ?
|
|
||
| setMenuItemDataAttributes(state); | ||
| // eslint-disable-next-line react-hooks/immutability | ||
| state.root['data-checked'] = stringifyDataAttribute(state.checked); |
There was a problem hiding this comment.
cant this be achieved just by calling the base hook and setting state.root ?
|
|
||
| export type { AvatarGroupContextValue, AvatarGroupContextValues, AvatarGroupSlots } from '@fluentui/react-avatar'; | ||
|
|
||
| export type AvatarGroupProps = AvatarGroupBaseProps; |
There was a problem hiding this comment.
re-export inline pls
| // eslint-disable-next-line react-hooks/immutability | ||
| state.root['data-disabled'] = stringifyDataAttribute(state.input.disabled); | ||
| // eslint-disable-next-line react-hooks/immutability | ||
| state.root['data-invalid'] = stringifyDataAttribute( |
There was a problem hiding this comment.
not sure if it makes sense to duplicate the attribute. we already have aria-invalid available
| /** | ||
| * Data attribute set when the link is rendered inline. | ||
| */ | ||
| 'data-inline'?: string; |
There was a problem hiding this comment.
isn't it a style prop leaked?
There was a problem hiding this comment.
good call, I think it was leaked, moreover, in current docs we have examples with no-op inline prop, styles are applied in the examples but independent from inline prop. I will omit it here and until we are stable we should remove that.
There was a problem hiding this comment.
| root: { | ||
| focusgroupstart?: string; | ||
| 'data-disabled'?: string; | ||
| 'data-has-submenu'?: string; |
There was a problem hiding this comment.
we have aria-has-popup and aria-expanded already available, do we need to duplicate those attributes?
There was a problem hiding this comment.
yes, good question, I am leaning towards keep these things separated, the opposite would help us to save some not significant amount of bytes on HTML size, but as we previously discussed it's relatively cheap stage of page parsing, so more HTML attributes on existing nodes won't hurt us. Keeping these things separated would have better separation of concerns (area for a11y, data-* for styling) and more obvious & expected DX
There was a problem hiding this comment.
some other DS following this path of separated aria and data attributes / duplications:
<input data-slot="input" class="..." id="input-invalid" placeholder="Error" aria-invalid="true" data-invalid="true">| root: { | ||
| 'data-disabled'?: string; | ||
| 'data-has-submenu'?: string; | ||
| 'data-submenu-open'?: string; |
| export type TooltipState = Omit<TooltipBaseState, 'mountNode' | 'hidden'>; | ||
| export type TooltipState = Omit<TooltipBaseState, 'mountNode' | 'hidden'> & { | ||
| content: { | ||
| 'data-visible'?: string; |
There was a problem hiding this comment.
we need to unify the data-visible/data-open naming convention
There was a problem hiding this comment.
agree, in my opinion, in most cases we should use data-open as it represents logical open / close state. data-visible can be used when visibility can differ from open / close (enter / exit transitions) and there are not so many cases as for data-open in the components. Here it was added because of parity with visible prop, but I do not think it should be the same as prop naming and it fits into the popover behaviors in other components.
There was a problem hiding this comment.
updated
| state.root['data-disabled'] = stringifyDataAttribute(state.textarea.disabled); | ||
| // eslint-disable-next-line react-hooks/immutability | ||
| state.root['data-invalid'] = stringifyDataAttribute( | ||
| state.textarea['aria-invalid'] === true || state.textarea['aria-invalid'] === 'true', |
There was a problem hiding this comment.
same as above
20545eb to
c1622bb
Compare
c1622bb to
eebc128
Compare
3 participants
data-*attributes.FUI Modern builds on headless components and needs stable selectors for state-driven styling: