Feat: Grid tiles automatically size to fill grid #186

hudson-newey · 2024-09-18T06:03:00Z

Feat: Grid tiles automatically size to fill grid

A short description of the pull request

Changes

Features

The verification grid now uses a display: grid layout
You can now specify the number of rows and columns in the verification grid through the --rows and --columns css variables (e.g. <oe-verification-grid style="--rows: 6; --collumns: 3;" /> (this comes for free and required no extra effort)
The verification grid no longer allows you to create a verification grid that would not fit inside the verification container
The verification grid now always fills its container
The verification controls are now always at the bottom of the oe-verification-grid element
The verification grid now has a default size for desktop and mobile
oe-verification-grid-tile components can now advertise if they are overflowing from the verification grid

Bug Fixes

Fix a bug where the axes svg would be larger than the spectrogram component, causing overflow
Replaces the intersection observer with a predicates to assert that the grid tiles will fit in the grid. This fixes all issues related to the intersection observer
When virtually measuring the axes component, it now adds padding to both sides of the label, better reflecting the real browser implementation. This fixes a bug where the axes labels could appear to be too close together (does not fix a similar font-measuring bug on Chrome)

Code Quality

The axes component's chrome is now overlayed on top of the wrapped element (spectrogram) instead of wrapping it. This allows the wrapped element/spectrogram to grow independently of the axes' size
Changes the oe-verification-grid's container to be a flex column instead of flex row. Better reflecting it's function of a container with elements of variable height
Inadvertently correctly fixes the bug in playwright that would cause the spectrograms to infinately expand. This was done because the oe-verification-grid container is now a flex column instead of flex row
Adds snapshot tests showing how the verification grid looks on different device types

Visual Changes

Screenshots of the verification grid at different device sizes can be found here

Related Issues

Fixes: #167
Fixes: #150
Fixes: #146
Fixes: #147
Fixes: #165

Final Checklist

Fixes: #167

github-actions · 2024-09-19T00:01:49Z

🚀 Deployed on https://66f60d03576d5e96353bb32b--oe-web-components.netlify.app

Previously axes elements would use absolute positioning, and the svg container would not effect sizing. This caused issues with dynamic grid tile sizes and caused the verification grid to overflow its container creating scrollbars on the host application

This reverts commit da7a414.

This reverts commit e324c2e.

atruskie

while reviewing I checked out the code and did some interactive testing. It's hard to describe what is going on, but on more than one occasion I managed to cause the tab to lock up (in i'm assuming what was an infinite loop of some kind).

We also need some indication to the user that their desired grid size is being ignored. I.e. if I set grid-size to 32 a but am only showed 12 items then that's confusing. One way to do this would be to set a maximum (and maybe issue a notice) on the grid-size ui. Another way could be to show the desired size and the current limit on the slider (again with a warning about not being able to show the requested amount).

This is some really great work, but we need another pass for reliability and edge cases.

src/components/axes/axes.ts

src/components/axes/css/style.css

src/components/verification-grid/verification-grid.ts

atruskie

Close. So close.

We talked offline about how this does not handle scenarios where the aspect ratio looks terrible, but the next candidate is not triggered because everything fits and target grid size in the only thing considered.

src/components/verification-grid-tile/css/style.css

atruskie · 2024-09-26T05:10:51Z

src/components/verification-grid-tile/verification-grid-tile.ts

+    this.intersectionObserver.observe(this.slotWrapper);
+    this.intersectionObserver.observe(this.contentsWrapper);


why observing both?

Discussed offline and added a code comment

src/helpers/controllers/dynamic-grid-sizes.ts

src/components/verification-grid/verification-grid.ts

src/helpers/controllers/dynamic-grid-sizes.ts

…tics/web-components into dynamic-grid-tile-sizes

hudson-newey self-assigned this Sep 18, 2024

hudson-newey added enhancement New feature or request Architecture ui labels Sep 18, 2024

hudson-newey and others added 15 commits September 19, 2024 09:50

Feat: Grid tiles automatically size to fill grid

a7c04f3

Fixes: #167

Fix axes

ec7fe03

Correct flexbox layout

b0da461

Add dynamic grid size rounding

fd5d301

Backfill unit tests for dynamic grid sizes

6c0260c

Staging commit

684d15f

Grid container now fills its host

f05cb97

Attempt to shrink before reducing grid size

40497f4

Vertically auto scale

850438d

Vertically autoscale spectrograms

3216600

Fully working version

57951eb

Prefer to shrink grid tiles

221bdf7

Implement new scaling algorithm

63d6eb5

Fix dev pages

dc3430c

Merge main

b1baefc

hudson-newey force-pushed the dynamic-grid-tile-sizes branch from 7f2940a to b1baefc Compare September 19, 2024 00:00

hudson-newey and others added 9 commits September 19, 2024 11:19

Revert "Axes elements now position theirself inside the svg"

72530d1

This reverts commit da7a414.

Correct axes svg

31fd74c

Add alternative grid shape algorithm

e324c2e

Revert "Add alternative grid shape algorithm"

52f77df

This reverts commit e324c2e.

Use flex column for grid

fd36f15

Stretch tiles & use flex column for grid container

058c2c7

Grid container no longer expands beyond max size

d848c1d

Correctly size grid

1c3d96d

atruskie requested changes Sep 23, 2024

View reviewed changes

hudson-newey added 13 commits September 24, 2024 14:07

Refactor grid size logic

b8d68a5

Revert hacky axes changes

6952f08

Add dynamic grid size class

7a0d4e4

New dynamic grid size algorithm

dbf191d

Got intersection observer working

c04bdeb

Staging

578d6e7

Correctly disconnect intersection observer

3b5decb

Add slot text tests

80a18e6

Update tests for dynamic grid tile sizes

a4e1c71

Update axes tests

dd2c168

Self review sub css changes

95522b8

CSS self review sub axes flex slot content

9dbf405

Fix axes css

b3a2be5

hudson-newey requested a review from atruskie September 26, 2024 04:48

hudson-newey added 2 commits September 26, 2024 15:32

Set verification grids default height to 100dvh

2a41e09

Add comments for intersection observer observations

8e62cd6

atruskie requested changes Sep 26, 2024

View reviewed changes

hudson-newey and others added 6 commits September 26, 2024 16:55

New overlapping invalidation without debounce

5f556d9

Add a different distance metric

4d392e0

Merge branch 'dynamic-grid-tile-sizes' of https://github.com/ecoacous…

f656d94

…tics/web-components into dynamic-grid-tile-sizes

Fix lint errors

99b880a

Simplify grid snapshot tests with hacky implementation

e3f4a16

Fix overflowing axes padding

b1b6222

hudson-newey requested a review from atruskie September 27, 2024 01:58

atruskie approved these changes Sep 27, 2024

View reviewed changes

hudson-newey merged commit 182224e into main Sep 27, 2024
6 checks passed

hudson-newey deleted the dynamic-grid-tile-sizes branch September 27, 2024 03:02

This was referenced Sep 27, 2024

chore(main): release web-components 0.2.0 #198

Closed

chore(main): release web-components 1.1.0 #199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Grid tiles automatically size to fill grid #186

Feat: Grid tiles automatically size to fill grid #186

hudson-newey commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 19, 2024 •

edited

Loading

atruskie left a comment

atruskie left a comment

atruskie Sep 26, 2024

hudson-newey Sep 26, 2024 •

edited

Loading

		this.intersectionObserver.observe(this.slotWrapper);
		this.intersectionObserver.observe(this.contentsWrapper);

Feat: Grid tiles automatically size to fill grid #186

Feat: Grid tiles automatically size to fill grid #186

Conversation

hudson-newey commented Sep 18, 2024 • edited Loading

Feat: Grid tiles automatically size to fill grid

Changes

Features

Bug Fixes

Code Quality

Visual Changes

Related Issues

Final Checklist

github-actions bot commented Sep 19, 2024 • edited Loading

atruskie left a comment

Choose a reason for hiding this comment

atruskie left a comment

Choose a reason for hiding this comment

atruskie Sep 26, 2024

Choose a reason for hiding this comment

hudson-newey Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

hudson-newey commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 19, 2024 •

edited

Loading

hudson-newey Sep 26, 2024 •

edited

Loading