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
Documentation: Tile #1538
Documentation: Tile #1538
Changes from 2 commits
87e470e
ea22cc5
1e36cc6
b7d98e0
2ad7434
6e0be32
7af5ea4
652a356
794b5a3
846049d
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.
What does it mean "Users can also take advantage of properties provided for Tile"? This doesn't really explain anything, as props are always available in every component.
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.
if it helps, you could look at expandable block documentation for ideas on how to separate composition and legacy apis. https://github.com/iTwin/iTwinUI/pull/1502/files#diff-19c324894ed0750aa99b244219e53d7e123753f73b594a0211986918f7bf4e03
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.
Despite the wording, what's wrong with my choice of documenting both 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.
The example is fine, but the wording just feels unhelpful in general.
"You can take advantage of props" is not some new revelation; users can always take advantage of props. It might help to describe these props or mention which ones.
But the key difference is between directly using
Tile
vs only using subcomponents. See "usage" section here: https://dev-itwinui.bentley.com/docs/expandableblock#usageThere 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, is it better now?
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 believe it would be helpful to have a separate section like the Expandable Block -> Usage link that Mayank shared. It can say something like, "you can use Tile in either one of two ways: ...".
This makes it clearer that we shouldn't mix the APIs. This also allows us to talk about the pros and cons of each API to help users decide which API to choose,
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.
2/3 find this approach "good enough".
Leaving as is for now.
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 mention why to use
href
in<Tile.Action>
instead of wrapping the whole tile in<a href="...">
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 a bit. Feel free to rewrite things if you want.