Add v1 Button migration and spec files #1157
Conversation
rurikoaraki
requested review from
ValentinaKozlova,
lenahong,
chiuam and
ejlayne
| - `children` | ||
| - `disabled` | ||
| - `icon` | ||
| - `onClick` |
There was a problem hiding this comment.
nit: I would vote to rename this onPress, to better match pressable, but that's a followup for me on my other code review #1070 anyway
There was a problem hiding this comment.
Ah I forgot about that conversation. I think this will be a case of balancing alignment with web vs. what is intuitively native/RN, since I think web is named onClick to align with the HTML API while onPress is used across RN. I think it's worth a discussion @ejlayne
| - `endIcon` => Use `right` value for `iconPosition` prop and pass icon information into `icon` prop instead | ||
| - `startIcon` => Use `left` value for `iconPosition` prop and pass icon information into `icon` prop instead |
There was a problem hiding this comment.
nit: I would say we pass start and end to iconPosition for better internationalization
There was a problem hiding this comment.
We can bring this feedback to web. @ejlayne
There was a problem hiding this comment.
Fair point, let's bring up with web folks.
There was a problem hiding this comment.
Similar platform difference that is a point of discussion
| ### Slots | ||
|
|
||
| - `root` - The outer container representing the `Button` itself that wraps everything passed via the `children` prop. | ||
| - `icon` - If specified, renders an `icon` either before or after the `children` as specified by the `iconPosition` prop. | ||
| - `loader` - If specified, renders a `loader` before the `icon` and `children` while the `loading` flag is set to `true`. |
There was a problem hiding this comment.
Nit: (and I'm opening a can of worms), I somewhat feel the public API should just be the props and tokens of a component, and not slots. That's mostly because slots tend to change as design evolves, which makes it harder to be part of the public API.
Granted, as things are now, clients have access to slots, so we should at least document them.
The general answer is, "we are trying to align with the web API to make implementation across fluentui and FURN a smoother experience." We apparently get a lot of feedback around how different the APIs are across the platforms, so while some of these are easy to accomplish, it would be a quick ease of use thing to add props surfacing some defaults. There are some props we're not carrying over due to inherit differences between web and native (namely accessibility) but we're aiming to make the porting process smoother since it's a common scenario for clients. |
| - `endIcon` => Use `right` value for `iconPosition` prop and pass icon information into `icon` prop instead | ||
| - `startIcon` => Use `left` value for `iconPosition` prop and pass icon information into `icon` prop instead |
There was a problem hiding this comment.
Fair point, let's bring up with web folks.
|
@ejlayne @Saadnajmi @chiuam Any other comments? Otherwise I will merge this at EOD. |
packages/experimental/Button/SPEC.md
Outdated
| // * Loader slot that, if specified, renders a `loader` before the `icon` and `children` while the `loading` flag | ||
| // * is set to `true`. | ||
| // */ | ||
| loader?: TBD; |
There was a problem hiding this comment.
TBD? The user will pass in the slot? Is that even possible?
There was a problem hiding this comment.
TBD as in I don't know the type of the loader slot yet since that was still in discussion. I think I will change this to be ActivityIndicator
There was a problem hiding this comment.
If we do that, I think it would be a simple change to have FURN ActivityIndicator fall back to the RN base ActivityIndicator for not iOS/Android. But that's a whole different thing =)
packages/experimental/Button/SPEC.md
Outdated
| #### Touch interaction | ||
|
|
||
| The same behavior as above translated for touch events. This means that there is no equivalent for `mouseenter` and `mouseleave`, which makes it so that the hovered state cannot be accessed. |
There was a problem hiding this comment.
This is part of why I want to push for #1070 to make this API a lot simpler. It would be great if we could just communicate that a Button is a Pressable, which supports PressableProps, which includes hover / keyboard focus / touch / click interaction. This last bit of documentation to me feels a bit too web and less RN.
I don't consider that blocking this review though, since this is all iteration.
|
@Saadnajmi Any other feedback before I merge? |
| - `endIcon` => Use `right` value for `iconPosition` prop and pass icon information into `icon` prop instead | ||
| - `startIcon` => Use `left` value for `iconPosition` prop and pass icon information into `icon` prop instead |
There was a problem hiding this comment.
Similar platform difference that is a point of discussion
Platforms Impacted
Description of changes
Adding documentation for v1 for spec and migration.
Shamelessly taking the layout from the fluentui repo.
The specs/migration steps are a first-pass level, nothing is finalized yet. Figured I'd get the documentation started so that it's easier to add to as we finalize things.
Pull request checklist
This PR has considered (when applicable):