Skip to content

fix(tile-group): keep selection icon size consistent for large tiles - #11215

Merged
jcfranco merged 6 commits into
devfrom
jcfranco/10931-keep-selection-icon-size-consistent-for-large-tiles
Jan 9, 2025
Merged

fix(tile-group): keep selection icon size consistent for large tiles#11215
jcfranco merged 6 commits into
devfrom
jcfranco/10931-keep-selection-icon-size-consistent-for-large-tiles

Conversation

@jcfranco

@jcfranco jcfranco commented Jan 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Related Issue: #10931

Summary

Fixes an issue where large tiles (w/o description) would cause the selection icon to be oversized and misaligned.

Noteworthy changes

  • refactors stories to use HTML-generating functions to improve maintainability
  • adds internal CSS var to reuse and simplify internal space assignments
  • applies icon styling separately
  • applies relative positioning to content container
  • replaces sticky z-index var in favor of default + bump to keep it within the same layer

@github-actions github-actions Bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Jan 7, 2025
@jcfranco jcfranco added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jan 7, 2025
macandcheese
macandcheese reviewed Jan 7, 2025
View reviewed changes
Comment thread packages/calcite-components/src/components/tile-group/tile-group.stories.ts
</div>
`;
export const allVariantsHorizontal = createVariantsHtmlStory("horizontal");
export const allVariantsVertical = createVariantsHtmlStory("vertical");

@macandcheese macandcheese Jan 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you broke Chromatic 😄 https://www.chromatic.com/test?appId=6266d45509d7eb004aa254fb&id=677ce7bbb76f8e6437051490

@jcfranco jcfranco Jan 8, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh noes! @eriklharper Would a shorter description value help here? We could split the all-variants stories, but it sort of defeats the purpose. 🥲

@eriklharper eriklharper Jan 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You'd have to just try shortening it I suppose and see it makes it small enough for Chromatic to capture, but yeah, possibly that could make a difference.

eriklharper
eriklharper reviewed Jan 8, 2025
View reviewed changes
Comment thread packages/calcite-components/src/components/tile/tile.scss Outdated
@eriklharper

eriklharper commented Jan 8, 2025

Copy link
Copy Markdown
Contributor

I'm seeing a hover border regression where the right side of the middle hovered tile doesn't appear correctly. Could be a stacking issue:

image

@jcfranco jcfranco 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 Jan 8, 2025
@jcfranco
jcfranco requested a review from eriklharper January 8, 2025 08:32
@jcfranco

jcfranco commented Jan 8, 2025

Copy link
Copy Markdown
Member Author

@eriklharper Fixed the hover outline regression. Would you mind taking another look?

@jcfranco
jcfranco requested a review from ashetland January 8, 2025 08:33
@eriklharper

eriklharper commented Jan 8, 2025

Copy link
Copy Markdown
Contributor

I'm seeing a hover border regression where the right side of the middle hovered tile doesn't appear correctly. Could be a stacking issue:

image

UPDATE: I'm no longer seeing this happen locally on your latest changes.

eriklharper
eriklharper approved these changes Jan 8, 2025
View reviewed changes

@eriklharper eriklharper left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NOICE! Thanks @jcfranco for addressing this! I definitely caused this to happen as part of my selection-mode changes to Tile, so appreciate the fix and the nicer story setup :)

@eriklharper eriklharper mentioned this pull request Jan 8, 2025
Merged
@jcfranco jcfranco 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 Jan 8, 2025
ashetland
ashetland approved these changes Jan 9, 2025
View reviewed changes

@ashetland ashetland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Visuals lookin' gewd!

@jcfranco
jcfranco merged commit de20dc8 into dev Jan 9, 2025
@jcfranco
jcfranco deleted the jcfranco/10931-keep-selection-icon-size-consistent-for-large-tiles branch January 9, 2025 00:09
jcfranco added a commit that referenced this pull request Jan 10, 2025
@jcfranco
refactor(tile): remove unused data attributes (#11216)
4fccea7 
**Related Issue:** N/A

## Summary

✨🧹✨

**Note**: #11215
should be merged prior to this.
benelan pushed a commit that referenced this pull request Feb 8, 2025
@jcfranco @benelan
fix(tile-group): keep selection icon size consistent for large tiles (#…
0f5d196 
…11215)

**Related Issue:** #10931 

## Summary

Fixes an issue where large tiles (w/o `description`) would cause the
selection icon to be oversized and misaligned.

### Noteworthy changes

* refactors stories to use HTML-generating functions to improve
maintainability
* adds internal CSS var to reuse and simplify internal space assignments
* applies icon styling separately
* applies relative positioning to content container
* replaces sticky z-index var in favor of default + bump to keep it
within the same layer
benelan pushed a commit that referenced this pull request Feb 8, 2025
@jcfranco @benelan
refactor(tile): remove unused data attributes (#11216)
dc2fd0d 
**Related Issue:** N/A

## Summary

✨🧹✨

**Note**: #11215
should be merged prior to this.
@calcite-admin calcite-admin mentioned this pull request Feb 8, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@macandcheese macandcheese macandcheese left review comments
@eriklharper eriklharper eriklharper approved these changes
@ashetland ashetland ashetland approved these changes

Assignees

@jcfranco jcfranco

Labels

bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.

Projects

None yet

Milestone

2025-02-06 - 3.0.0 Feb Breaking Chang...

Development

Successfully merging this pull request may close these issues.

4 participants

@jcfranco @eriklharper @macandcheese @ashetland