Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[useRenderer] Add public hook & Slot component #1418
base: master
Are you sure you want to change the base?
[useRenderer] Add public hook & Slot component #1418
Changes from 11 commits
3cfc3c5
9f32729
1b705fa
5e4fbce
b7c1d09
8038e58
0051604
29fd26a
b87aadc
116a673
d50077d
3c753fa
2baa3dd
b94b8ce
6a8d16f
2a9621f
c523e6f
ec02061
034b747
f067410
7dcdc5e
15728ad
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'm not the biggest fan of this prop name (
styleHookMapping
) - not sure if "style hook" is suitable for a public API, andMap
instead ofMapping
might be better. It's not clear it infers/uses thestate
objectThere 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 most clear name would be:
stateToDataAttributesMap
, to be very explicit, but it's a strange name 😅 @aarongarciah or @colmtuite do you have some suggestions?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.
Yeah, "hook" has a strong different meaning in React world, so we can think of something else.
stateAttributes
?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.
stateDataAttributes
?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.
Technically, you could create any attribute (including
class
), so "dataAttributes" is not entirely correct. We just happen to use only data 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 see!
stateAttributes
makes sense then.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.
Renamed to
stateDataAttributes
, I am still missing the mapping in it, but people can figure out how to define them by using the type. Maybe it's even worth adding demo about it on the page. What do you think?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 isn't really the state of the component but its props, so someone might be confused about the purpose of State and style hooks. Can we introduce something else that doesn't come from props? For example, a character counter?
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.
export *
should be enough