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

fix(#7623): Resize ConductorAxis properly #7624

Merged
merged 37 commits into from
Mar 26, 2024
Merged

fix(#7623): Resize ConductorAxis properly #7624

merged 37 commits into from
Mar 26, 2024

Conversation

ozyx
Copy link
Contributor

@ozyx ozyx commented Mar 20, 2024

Closes #7623

Describe your changes:

  • Registers ResizeObserver for the ConductorAxis correctly using composables
  • Adds 3 Playwright snapshot tests for the ConductorAxis

e2e Framework Changes:

  • Overloads page.screenshot() passing in defaults that may apply to most Open MCT snapshot tests (mask timestamps, disable animations). These can be overridden in the test by passing an options object as normal.
  • Provides a new fixture tick() that is meant to be used in conjunction with overrideClock. This allows tests that have paused clock (shouldAdvanceTime: false) to be able to manually tick the clock to trigger ui updates, router params updates, etc.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this a notable change that will require a special callout in the release notes? For example, will this break compatibility with existing APIs or projects that consume these plugins?

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Has this been smoke tested?
  • Have you associated this PR with a type: label? Note: this is not necessarily the same as the original issue.
  • Have you associated a milestone with this PR? Note: leave blank if unsure.
  • Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

  • Changes appear to address issue?
  • Reviewer has tested changes by following the provided instructions?
  • Changes appear not to be breaking changes?
  • Appropriate automated tests included?
  • Code style and in-line documentation are appropriate?

@ozyx ozyx added the type:bug label Mar 20, 2024
@ozyx ozyx added this to the Target:4.0.0 milestone Mar 20, 2024
@ozyx ozyx requested review from akhenry and scottbell March 20, 2024 00:28
Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 31.03448% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 56.19%. Comparing base (1d5ddc5) to head (f13d85b).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7624      +/-   ##
==========================================
+ Coverage   56.14%   56.19%   +0.04%     
==========================================
  Files         672      672              
  Lines       27128    27133       +5     
  Branches     2635     2635              
==========================================
+ Hits        15231    15247      +16     
+ Misses      11569    11559      -10     
+ Partials      328      327       -1     
Flag Coverage Δ
e2e-full 23.67% <ø> (+0.09%) ⬆️
e2e-stable 59.97% <ø> (+0.06%) ⬆️
unit 49.13% <31.03%> (+0.02%) ⬆️
Files Coverage Δ
src/plugins/timeConductor/ConductorAxis.vue 26.85% <31.03%> (+1.60%) ⬆️

... and 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d5ddc5...f13d85b. Read the comment docs.

Copy link
Contributor

@scottbell scottbell left a comment

Choose a reason for hiding this comment

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

Had a nitpick, but nothing to stop this. Nice work!

Before:

before.mov

After:

after.mov

@@ -55,20 +57,69 @@ export default {
}
},
emits: ['pan-axis', 'end-pan', 'zoom-axis', 'end-zoom'],
setup() {
const axisHolder = ref(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

this all looks very familiar 😀

src/plugins/timeConductor/ConductorAxis.vue Outdated Show resolved Hide resolved
@scottbell scottbell added the pr:e2e:couchdb npm run test:e2e:couchdb label Mar 20, 2024
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Mar 20, 2024
@ozyx
Copy link
Contributor Author

ozyx commented Mar 20, 2024

Wait to merge, have a visual test inc

@scottbell
Copy link
Contributor

Wait to merge, have a visual test inc

Yeah, figured. I was going to suggest that but my brain shorted.

@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 45.94595% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 56.30%. Comparing base (7e926cc) to head (ce0fdea).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7624      +/-   ##
==========================================
- Coverage   56.31%   56.30%   -0.01%     
==========================================
  Files         672      672              
  Lines       27114    27120       +6     
  Branches     2632     2632              
==========================================
+ Hits        15269    15270       +1     
- Misses      11517    11522       +5     
  Partials      328      328              
Flag Coverage Δ
e2e-full 23.64% <ø> (+0.09%) ⬆️
e2e-stable 60.36% <ø> (+<0.01%) ⬆️
unit 49.09% <45.94%> (-0.01%) ⬇️
Files Coverage Δ
src/plugins/summaryWidget/src/Rule.js 87.33% <ø> (ø)
...rc/plugins/summaryWidget/src/input/ColorPalette.js 100.00% <ø> (ø)
src/plugins/summaryWidget/src/input/IconPalette.js 94.44% <ø> (ø)
src/ui/components/List/ListItem.vue 100.00% <ø> (ø)
src/utils/testing.js 76.41% <100.00%> (ø)
src/plugins/timeConductor/ConductorAxis.vue 27.52% <33.33%> (+2.28%) ⬆️

... and 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e926cc...ce0fdea. Read the comment docs.

Copy link
Collaborator

@unlikelyzero unlikelyzero left a comment

Choose a reason for hiding this comment

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

This is great! Just two changes

  • add framework tests for the tick functionality
  • Update the new visual test to use an a11y check in aftereach
  • update the readme to instruct Windows users to use the Linux container snapshot method

e2e/avpFixtures.js Show resolved Hide resolved
e2e/baseFixtures.js Show resolved Hide resolved
e2e/tests/framework/baseFixtures.e2e.spec.js Show resolved Hide resolved
karma.conf.cjs Show resolved Hide resolved
Copy link
Collaborator

@unlikelyzero unlikelyzero left a comment

Choose a reason for hiding this comment

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

Unless I'm missing it on mobile, we're still not executing the accessibility checks

expect(time).toBe(MISSION_TIME + 1000 * 1);
await tick(1000);
time = await page.evaluate(() => new Date().getTime());
expect(time).toBe(MISSION_TIME + 1000 * 2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome

@ozyx ozyx enabled auto-merge (squash) March 26, 2024 21:01
@ozyx ozyx added the pr:e2e:couchdb npm run test:e2e:couchdb label Mar 26, 2024
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Mar 26, 2024
@ozyx ozyx added the pr:e2e:couchdb npm run test:e2e:couchdb label Mar 26, 2024
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Mar 26, 2024
@ozyx ozyx merged commit 5b4ee19 into master Mar 26, 2024
19 checks passed
@ozyx ozyx deleted the mct7623 branch March 26, 2024 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Time Conductor Axis does not resize correctly in certain scenario
5 participants