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

Test scrollbar is ignored in width constraints (#762) #763

Merged
merged 2 commits into from
Sep 29, 2021

Conversation

ctjhoa
Copy link
Contributor

@ctjhoa ctjhoa commented Sep 11, 2019

#762
This is a draft PR which doesn't include tests.
It's mostly to start discussion around the solution and the global design.

addon/-private/column-tree.js Outdated Show resolved Hide resolved
@ctjhoa ctjhoa force-pushed the fix_container_width_with_scrollbar branch from df8e23d to eacff06 Compare September 11, 2019 15:54
@ctjhoa
Copy link
Contributor Author

ctjhoa commented Sep 11, 2019

This has been manually tested on Edge, Firefox, Chrome and Safari.

@ctjhoa ctjhoa force-pushed the fix_container_width_with_scrollbar branch from bf5867d to f8495c7 Compare September 11, 2019 16:05
@ctjhoa ctjhoa force-pushed the fix_container_width_with_scrollbar branch 3 times, most recently from 1994d1b to 95ee603 Compare October 31, 2019 15:09
Copy link
Member

@mixonic mixonic left a comment

Choose a reason for hiding this comment

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

I think avoiding the next( is a bit tricky and may require some deep diving, but we should do that before landing this fix.

Thanks for the spike! I think test cases or understanding how this behaves in which browsers (and with what kinds of devices attached 😢) would be helpful.

addon/-private/column-tree.js Outdated Show resolved Hide resolved
scheduler.schedule(
'affect',
() => {
next(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Why is next needed here?

This seems not ideal. The logic seems to be: When wrappedRows changes the UI will reflow, so the width constraints needs to be revalidated. There must already be something doing that though, or some other mechanism we can use besides an observer to make it happen. This is definitely a tricky bit.

@ctjhoa ctjhoa force-pushed the fix_container_width_with_scrollbar branch 2 times, most recently from e521327 to 557c94c Compare November 21, 2019 16:23
@ctjhoa
Copy link
Contributor Author

ctjhoa commented Nov 21, 2019

@mixonic
Hi, I've had some times to work on this.
So I have now a better understanding of the sizes computations but correct me if I'm wrong.
Browsers aren't consistent in their way to expose sizes of an element. For example, they expose offsetWidth as integer and getBoundingClientRect().width as float. Unfortunately, there isn't always an integer/float equivalent exposed.
scale seems to be a ratio which allows to go between integer and float values.

My first version had wrong assumptions on those size types and that's why I needed to round the scale to get accurate result. This should be fixed now.

I've added tests too to demonstrate the initial issue. However, I didn't change the observer yet. This will be my future task. Anyway it was just to give you updated status.

@ctjhoa ctjhoa force-pushed the fix_container_width_with_scrollbar branch from 557c94c to fd83fae Compare August 5, 2021 15:47
@ctjhoa
Copy link
Contributor Author

ctjhoa commented Aug 5, 2021

@mixonic
Sorry for the late reply
Test is ok on latest version of ember-table so this PR only add a test to cover this specific case.

@ctjhoa ctjhoa requested a review from mixonic August 5, 2021 15:48
@ctjhoa ctjhoa changed the title [FEAT] Ignore scrollbar in width constraints (#762) Test scrollbar is ignored in width constraints (#762) Aug 5, 2021
@mixonic
Copy link
Member

mixonic commented Sep 15, 2021

@ctjhoa Do you think this issue is only relevant to older Ember versions? I'm surprised this test would only fail for 2.8 and 2.12.

If you want to set up some time to pair or discuss please shoot me an email (address is on my profile) I'd happily set up the time.

@ctjhoa ctjhoa force-pushed the fix_container_width_with_scrollbar branch from b0d82f6 to fa73f30 Compare September 22, 2021 06:59
@ctjhoa
Copy link
Contributor Author

ctjhoa commented Sep 22, 2021

@mixonic
Older Ember versions timeout when rendering a high number of rows.
Decreasing the number of generated rows fix the issue on older Ember versions.
Probably due to an old rendering engine.

Tests are successful except ember-beta & ember-canary which fail but caused by those changes.

------ RESULTS ------

Scenario ember-lts-2.8: SUCCESS
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-compatibili… │ 1.2.1              │ 1.2.1                        │ yarn     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ Not Installed      │ Not Installed                │ yarn     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-angle-brack… │ Not Installed      │ Not Installed                │ yarn     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ bower              │ ^1.8.2             │ 1.8.12                       │ yarn     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember              │ components/ember#… │ 2.8.3+c4330341               │ bower    │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

DEPRECATION: The next major version of `ember-try` (v2.0) will drop support for installing dependencies with `bower`.
Scenario ember-lts-2.12: SUCCESS
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ ~2.12.0            │ 2.12.2                       │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-lts-2.18: SUCCESS
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ ~2.18.0            │ 2.18.2                       │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-lts-3.4: SUCCESS
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ ~3.4.0             │ 3.4.8                        │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-lts-3.8: SUCCESS
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ ~3.8.0             │ 3.8.3                        │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-lts-3.12: SUCCESS
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ ~3.12.0            │ 3.12.4                       │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-lts-3.16: SUCCESS
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ ~3.16.0            │ 3.16.10                      │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-lts-3.20: SUCCESS
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ ~3.20.0            │ 3.20.7                       │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-lts-3.24: SUCCESS
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ ~3.24.0            │ 3.24.5                       │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-release: SUCCESS
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ https://s3.amazon… │ 3.28.0-release+f243a4f0      │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-beta: FAIL
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ https://s3.amazon… │ 4.0.0-beta.4.beta+52c81a56   │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-canary: FAIL
Command run: ember test
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-source       │ https://s3.amazon… │ 4.1.0-alpha.1.canary+2d1033… │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘

Scenario ember-default: SUCCESS
Command run: ember test
Scenario ember-production: SUCCESS
Command run: ember test -e production
Scenario ember-default-docs: SUCCESS
Command run: ember test --filter="Acceptance | docs"
┌────────────────────┬────────────────────┬──────────────────────────────┬──────────┐
│ Dependency         │ Expected           │ Used                         │ Type     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-data         │ ~3.24.0            │ 3.24.2                       │ yarn     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-cli-addon-d… │ ^1.0.0             │ 1.0.0                        │ yarn     │
├────────────────────┼────────────────────┼──────────────────────────────┼──────────┤
│ ember-cli-addon-d… │ ^1.0.0             │ 1.0.0                        │ yarn     │
└────────────────────┴────────────────────┴──────────────────────────────┴──────────┘


2 scenarios failed
13 scenarios succeeded
15 scenarios run

@mixonic mixonic force-pushed the fix_container_width_with_scrollbar branch from fa73f30 to 658838e Compare September 29, 2021 14:36
@mixonic mixonic merged commit b6e5951 into Addepar:master Sep 29, 2021
@ctjhoa ctjhoa deleted the fix_container_width_with_scrollbar branch September 29, 2021 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants