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

Tile composition #1255

Merged
merged 97 commits into from
Jun 28, 2023
Merged

Tile composition #1255

merged 97 commits into from
Jun 28, 2023

Conversation

LostABike
Copy link
Contributor

@LostABike LostABike commented May 8, 2023

Changes

Convert most Tile's props into polymographic subcomponents with the following structure:

    <Tile>
      <Tile.ThumbnailArea>
        <Tile.ThumbnailPicture/>
        <Tile.Badge/>
        <Tile.TypeIndicator/>
        <Tile.QuickAction/>
      </Tile.ThumbnailArea>
      <Tile.Name/>
      <Tile.ContentArea>
        <Tile.Description />
        <Tile.Metadata/>
        <Tile.MoreOptions/>
      </Tile.ContentArea>
      <Tile.Buttons/>
    </Tile>

Testing

  • Updated all Storybook.
  • Updated all unit tests.

Docs

  • Included comments for each subcomponents export. Some have example.
  • Created example code, but documentation page .mdx might need to be in a separate PR since behaviors differ from Storybook on certain code.

@LostABike LostABike mentioned this pull request May 8, 2023
24 tasks
@gretanausedaite gretanausedaite added this to the React 3.0 milestone May 8, 2023
@LostABike LostABike requested a review from mayank99 May 8, 2023 16:31
@LostABike
Copy link
Contributor Author

@mayank99 can you briefly check if I'm going the right direction? before I try to complete the other subcomponents. Also I made a different file to work on incrementally rather than modifying existing Tile, easier for me this way. Once everything works right I'll deleted old Tile and rename new Tile

@LostABike LostABike changed the title Tile composition draft Tile composition May 16, 2023
@LostABike LostABike marked this pull request as ready for review May 16, 2023 20:45
@LostABike LostABike requested a review from a team as a code owner May 16, 2023 20:45
@LostABike LostABike requested review from a team and adamhock and removed request for a team May 16, 2023 20:45
@mayank99 mayank99 changed the base branch from main to dev May 17, 2023 19:05
Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to review this in more detail tomorrow. For now, my only comment is to suggest making use of the polymorphic utility from #1279 (see example below).

@mayank99
Copy link
Contributor

The folder variant is relying on the name prop to make its own iui-tile-name within iui-tile-content, should we still keep the name prop and pass it around the subcomponents? Or restructure css to have to have name outside of content even for folder variant?

We can restructure the html/css. It was kept this way in #1125 for backwards compatibility.

Copy link
Contributor

@adamhock adamhock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nitpicky comment. Other than that, make sure the tests pass before you merge!

Fantastic job! 🎉

*/
export const Tile = Object.assign(TileComponent, {
/**
*ThumbnailArea subcomponent that contains `ThumbnailPicture`, `QuickAction`, `TypeIndicator` or `BadgeContainer`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a space before ThumbnailArea on this line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 😄 , and thanks!

@LostABike
Copy link
Contributor Author

@FlyersPh9 do you want to make a last check on the css/html of this? before I merge (if I manage to pass those image tests that is 😂 )

Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice work

@LostABike LostABike added this pull request to the merge queue Jun 28, 2023
Merged via the queue into dev with commit e73eec4 Jun 28, 2023
@LostABike LostABike deleted the tile-composition branch June 28, 2023 18:44
@mayank99 mayank99 mentioned this pull request Aug 30, 2023
@imodeljs-admin imodeljs-admin mentioned this pull request Oct 23, 2023
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.

5 participants