-
Notifications
You must be signed in to change notification settings - Fork 42
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
Tile: add back old api #1527
Tile: add back old api #1527
Conversation
<MenuItem key='menuitem-1'>Item 2</MenuItem>, | ||
]} | ||
leftIcon={ | ||
<Tile.IconButton> |
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.
why IconButton
doesn't work here?
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.
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.
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.
for now i would say use IconButton
with manual size='small'
in this unit test. we don't want to mix and match old and new apis.
the color issue can be fixed in a separate PR
* Default tile variant or the folder layout. | ||
* @default 'default' | ||
*/ | ||
variant?: 'default' | 'folder'; |
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.
does folder tile work correctly? i think we had some issues supporting both variants with old apis
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.
just tested in the playground and it's working correctly, even with onClick
.
looks like it was fixed in #1255 (comment) + #1255 (comment)
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.
Would be good if there are some examples using the old Tile API. (Similar to #1502 (review))
examples/Tile.main.tsx
and the other Tile examples that are not currently in the PR are giving type errors. I believe Tile
should be changed to Tile.Wrapper
for the other Tile examples where we want to demo the new API.
i've updated the missed references. i'll leave it on Greta to handle the rest |
@gretanausedaite I noticed you added this to the merge queue. Are we not adding examples using the old Tile API to the website? |
Created #1538 |
Changes
Adding back props API. Now Tile works both ways:
Testing
Added unit test
Docs
Changeset