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

feat(tile-group): add responsive Tile Group component #8687

Merged
merged 14 commits into from
Feb 20, 2024

Conversation

eriklharper
Copy link
Contributor

@eriklharper eriklharper commented Feb 2, 2024

Related Issues: #8615 #6691 #6662

Summary

This PR adds the new calcite-tile-group component. It includes these responsiveness features that address #6691:

  • Wrapped Tiles match width of Tiles above
  • Wrapped Tiles match height of tallest Tile in the group

The changes in this PR were extracted from and depend on the changes in #8681.

@eriklharper eriklharper requested a review from a team as a code owner February 2, 2024 00:04
@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Feb 2, 2024
eriklharper added a commit that referenced this pull request Feb 2, 2024
Copy link
Contributor

@macandcheese macandcheese left a comment

Choose a reason for hiding this comment

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

Nice! Some things I saw in local testing:

This hover state needs some adjustment:

Screenshot 2024-02-01 at 4 10 52 PM

I'm also unable to see a focus outline when focused - the Tile is focused as I can navigate with keyboard "Enter" when href is present but there isn't a visual indication.

Should there be keyboard support for arrow / home / end within these groups? If so, would it change based on layout?

@eriklharper eriklharper changed the title feat(tile-group): add Tile Group component feat(tile-group): add Tile Group component with responsive layout Feb 2, 2024
@eriklharper
Copy link
Contributor Author

Should there be keyboard support for arrow / home / end within these groups? If so, would it change based on layout?

This PR is just about adding responsive layout features. I intend on addressing this in follow-up PRs tied to the #6662 epic.

@eriklharper
Copy link
Contributor Author

eriklharper commented Feb 2, 2024

This hover state needs some adjustment:

Screenshot 2024-02-01 at 4 10 52 PM I'm also unable to see a focus outline when focused - the Tile is focused as I can navigate with keyboard "Enter" when `href` is present but there isn't a visual indication.

This is also out of scope for this PR. These issues are the responsibility of Tile, which will come in subsequent PRs after this initial PR to add tile-group is merged in.

@eriklharper eriklharper added the low risk Issues with low risk for consideration in low risk milestones label Feb 2, 2024
@eriklharper eriklharper added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Feb 3, 2024
@eriklharper eriklharper added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Feb 3, 2024
@eriklharper eriklharper added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Feb 6, 2024
@eriklharper eriklharper added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Feb 7, 2024
@eriklharper eriklharper changed the title feat(tile-group): add Tile Group component with responsive layout feat(tile-group): add responsive Tile Group component Feb 15, 2024
@eriklharper
Copy link
Contributor Author

eriklharper commented Feb 17, 2024

Nice! Some things I saw in local testing:

This hover state needs some adjustment:

Screenshot 2024-02-01 at 4 10 52 PM

This has been fixed on this branch. Thanks for the suggestion!

@eriklharper eriklharper added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Feb 17, 2024
Copy link
Contributor

@macandcheese macandcheese left a comment

Choose a reason for hiding this comment

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

Looks good!

(design) - should there be a hover state that is distinct from focus state? Right now they are the same cc @SkyeSeitz @ashetland

@alisonailea alisonailea changed the base branch from main to epic/7180-component-tokens February 20, 2024 20:06
@eriklharper eriklharper merged commit bf0c906 into epic/7180-component-tokens Feb 20, 2024
17 checks passed
@eriklharper eriklharper deleted the eriklharper/8615-tile-group branch February 20, 2024 21:18
@eriklharper eriklharper restored the eriklharper/8615-tile-group branch February 22, 2024 00:33
alisonailea added a commit that referenced this pull request Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues tied to a new feature or request. low risk Issues with low risk for consideration in low risk milestones pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants