Skip to content
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

Merged
merged 10 commits into from
Sep 5, 2023
Merged

Documentation: Tile #1538

merged 10 commits into from
Sep 5, 2023

Conversation

gretanausedaite
Copy link
Contributor

Changes

Added documentation of the Tile.

Testing

N/A

Docs

N/A

@gretanausedaite gretanausedaite requested review from a team as code owners August 31, 2023 11:20
@gretanausedaite gretanausedaite requested review from mayank99 and r100-stack and removed request for a team August 31, 2023 11:20
@gretanausedaite gretanausedaite self-assigned this Aug 31, 2023
<AllExamples.TileActionableExample client:load />
</LiveExample>

Tile can also link to another page.
Copy link
Member

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="...">

Copy link
Contributor Author

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.


### Props API

Users can also take advantage of properties provided for `Tile`. A list of available properties can be found [at the end](#props) of this documentation.
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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#usage

Copy link
Contributor Author

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?

Copy link
Member

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,

Copy link
Contributor Author

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.

@gretanausedaite gretanausedaite added this pull request to the merge queue Sep 5, 2023
@mayank99 mayank99 removed this pull request from the merge queue due to a manual request Sep 5, 2023
@mayank99 mayank99 merged commit 1f1addc into dev Sep 5, 2023
@mayank99 mayank99 deleted the greta/website-updates branch September 5, 2023 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants