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

VUU21: Implement the screenshot functionality into the Save Layout component #23

Conversation

cfisher-scottlogic
Copy link

@cfisher-scottlogic cfisher-scottlogic commented Aug 18, 2023

Scope

VUU21

This incorporates the work done in the screenshot spike (#19) by adding screenshot functionality into the save dialog component.

Implementation

Screenshot functionality is provided with the html-to-image package in screenshot-utils.ts. The screenshot is currently stored in browser (local) storage as a base64 encoded string. Where fonts are imported externally via a link, the crossorigin="anonymous" tag has been added due to CORS issues, see this stackoverflow post and this html-to-image GitHub issue discussing the issue.

A unit test has been written for takeScreenshot(). The html-to-image package had to be mocked out as the browser operations it relies on are not available in the test environment (neither happy-dom nor jsdom).

A Cypress e2e test has been written to check a screenshot can be taken and rendered successfully from the UI. I've restructured the directory by separating the ./support files relevant to component & e2e tests.

Screenshots

image

@cfisher-scottlogic cfisher-scottlogic self-assigned this Aug 18, 2023
@cfisher-scottlogic cfisher-scottlogic changed the base branch from feature/VUU-17-layoutSave to feature/layout-management/layout-management-feature August 23, 2023 06:31
vuu-ui/.eslintrc.json Outdated Show resolved Hide resolved
Comment on lines 11 to 25
// TODO: Give the button and tab an accessible selector
cy.findByRole("tab", { name: "My Instruments" }).then((tab) => {
cy.wrap(tab).findByRole("button").click();
});

// Click the save layout button
// TODO: Can this be more accessible?
cy.findByRole("menuitem", { name: "Save Layout" }).click();

// Check the screenshot is displayed
// TODO: Don't find by classname, use an accessible selector
cy.get(".vuuSaveLayoutPanel").then((dialog) => {
cy.wrap(dialog)
.find("img")
.should("be.visible")
.and(($img) => {
expect($img[0].naturalWidth).to.be.greaterThan(0);
Copy link
Author

@cfisher-scottlogic cfisher-scottlogic Aug 23, 2023

Choose a reason for hiding this comment

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

I'm going to explore alternative selector options to hopefully land on something which is more indicative of what a user would actually do to make it more accessible.

Copy link
Author

@cfisher-scottlogic cfisher-scottlogic Aug 25, 2023

Choose a reason for hiding this comment

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

We're wrapping up improvements to the e2e testing strategy (selectors, POM, etc) into a separate ticket as it'll be more of an ongoing effort. I've given the TODO comments ticket numbers to track it: 01d099d

vuu-ui/cypress/tests/checkAccessibility.tsx Outdated Show resolved Hide resolved
vuu-ui/showcase/src/examples/Apps/NewTheme.examples.tsx Outdated Show resolved Hide resolved
vuu-ui/showcase/src/examples/Layout/SavePanel.examples.tsx Outdated Show resolved Hide resolved
vuu-ui/showcase/src/examples/Layout/index.ts Outdated Show resolved Hide resolved
@cfisher-scottlogic
Copy link
Author

We need to decide what the base of this PR should be.

As this is adding functionality to VUU-17-layoutSave it makes sense to make that the base branch.

But, if this (VUU21) is merged into that branch (VUU17) before VUU17 is merged into the feature branch, it could really bloat the PR for VUU17 and make it harder to review non-screenshot related changes.

So we can either wait till VUU17 is merged into the feature branch before merging this PR in. Or we can merge this PR straight into the feature branch.

@cfisher-scottlogic cfisher-scottlogic linked an issue Aug 23, 2023 that may be closed by this pull request
@cfisher-scottlogic
Copy link
Author

We need to decide what the base of this PR should be.

As this is adding functionality to VUU-17-layoutSave it makes sense to make that the base branch.

But, if this (VUU21) is merged into that branch (VUU17) before VUU17 is merged into the feature branch, it could really bloat the PR for VUU17 and make it harder to review non-screenshot related changes.

So we can either wait till VUU17 is merged into the feature branch before merging this PR in. Or we can merge this PR straight into the feature branch.

Discussed offline. Base branch will be VUU-17-layoutSave for reviewing but should be merged into the feature branch when #17 has been merged.

@cfisher-scottlogic cfisher-scottlogic changed the base branch from feature/layout-management/layout-management-feature to feature/VUU-17-layoutSave August 23, 2023 10:21
@cfisher-scottlogic cfisher-scottlogic force-pushed the feature/layout-management/VUU21-save-layout-screenshot branch from f116bec to f7d4f53 Compare August 23, 2023 10:49
@cfisher-scottlogic cfisher-scottlogic marked this pull request as ready for review August 23, 2023 10:53
@cfisher-scottlogic cfisher-scottlogic force-pushed the feature/layout-management/VUU21-save-layout-screenshot branch from 01d099d to ad6e272 Compare August 25, 2023 15:26
@cfisher-scottlogic cfisher-scottlogic force-pushed the feature/layout-management/VUU21-save-layout-screenshot branch from f1f951e to ba5b5fb Compare August 29, 2023 11:11
Copy link

@pling-scottlogic pling-scottlogic left a comment

Choose a reason for hiding this comment

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

Just a couple of minor points about indentation. I'll leave it to someone with more JS/TS experience for approval.

vuu-ui/showcase/templates/index-preview.html Outdated Show resolved Hide resolved
vuu-ui/showcase/templates/index.html Outdated Show resolved Hide resolved
Base automatically changed from feature/VUU-17-layoutSave to feature/layout-management/layout-management-feature September 1, 2023 08:39
@vferraro-scottlogic vferraro-scottlogic merged commit 8909696 into feature/layout-management/layout-management-feature Sep 6, 2023
6 checks passed
@cfisher-scottlogic cfisher-scottlogic deleted the feature/layout-management/VUU21-save-layout-screenshot branch September 13, 2023 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hook up Save Layout component with screenshot feature
4 participants