Skip to content

fix(#7055): Expand on image with double click #7308

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Jan 14, 2024
Merged

fix(#7055): Expand on image with double click #7308

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
59d3573
Add double click event to expand image view
marcelo-earth Dec 16, 2023
8bede7b
Add performance tests for large view and close
marcelo-earth Dec 17, 2023
444e504
Add example imagery in flexible layout to test double-click functiona…
marcelo-earth Dec 19, 2023
3d3227f
Remove click event listener in ImageryView.vue
marcelo-earth Dec 19, 2023
f0a25e5
Move test to 'Example Imagery in Flexible layout'
marcelo-earth Dec 20, 2023
9171c89
Refactor exampleImagery.e2e.spec.js
marcelo-earth Dec 20, 2023
a52482e
Remove unnecessary code for opening and closing
marcelo-earth Dec 22, 2023
c4a286b
Replace pageSelector with getByRole on double-click image test
marcelo-earth Jan 8, 2024
12d6726
Add role and aria-label to focused image
marcelo-earth Jan 8, 2024
c2270ab
Merge branch 'master' into 7055-add-expand-with-double-click
ozyx Jan 8, 2024
590d94e
Remove unnecessary code and role attribute from ImageryView.vue
marcelo-earth Jan 10, 2024
bc04900
Merge branch 'master' into 7055-add-expand-with-double-click
ozyx Jan 10, 2024
f0dbf70
Merge branch 'master' into 7055-add-expand-with-double-click
ozyx Jan 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions e2e/tests/functional/plugins/imagery/exampleImagery.e2e.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,36 @@ test.describe('Example Imagery in Time Strip', () => {
// Navigate to timestrip
await page.goto(timeStripObject.url);
});

test.describe('Example Imagery in Flexible layout', () => {
let flexibleLayout;
test.beforeEach(async ({ page }) => {
await page.goto('./', { waitUntil: 'domcontentloaded' });

flexibleLayout = await createDomainObjectWithDefaults(page, { type: 'Flexible Layout' });

// Create Example Imagery inside the Flexible Layout
await createDomainObjectWithDefaults(page, {
type: 'Example Imagery',
parent: flexibleLayout.uuid
});

// Navigate back to Flexible Layout
await page.goto(flexibleLayout.url);
});

test('Can double-click on the image to view large image', async ({ page }) => {
// Double-click on the image to open large view
await page.locator('.image-wrapper').dblclick();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here. Is there a way we could avoid .locator() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! But I'm not sure if I did it right, would waitForSelector('.image-wrapper') work? If not, what would you recommend me to do (or use as the method to get the element)?

Copy link
Contributor

Choose a reason for hiding this comment

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

We're moving away from page.locator() in general and we now prefer to use ARIA based methods such as page.getByRole(). One way we could improve this could be by adding an accessible name (such as an aria-label to the img element. We could then update the locator to something like this:

await page.getByRole('img', { name: 'Focused Image' });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I have replaced the pageSelector of the image wrapper and the background image with getByRole.


// Check if the large view is visible
await page.waitForSelector('.c-imagery__main-image__bg', { state: 'visible' });

// Close the large view
await page.locator('[aria-label="Close"]').click();
});
});

test('Clicking a thumbnail loads the image in large view', async ({ page, browserName }) => {
test.info().annotations.push({
type: 'issue',
Expand Down
8 changes: 8 additions & 0 deletions e2e/tests/performance/contract/imagery.contract.perf.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,14 @@ test.describe('Performance tests', () => {
await page.locator('button:has-text("Large View")').click(); //This action includes the performance.mark named 'viewLarge.start'
await page.evaluate(() => window.performance.mark('viewLarge.start.test')); //This is a mark only to compare evaluate timing

// Click Close Icon to return to normal view
await page.locator('[aria-label="Close"]').click();
await page.evaluate(() => window.performance.mark('view-large-close-button'));

// Open Large view by double-clicking on the image wrapper
await page.locator('.image-wrapper').dblclick(); //This action includes the performance.mark named 'viewLarge.start'
await page.evaluate(() => window.performance.mark('viewLarge.start.test')); //This is a mark only to compare evaluate timing

//Time to Imagery Rendered in Large Frame
await page.waitForSelector('.c-imagery__main-image__bg', { state: 'visible' });
await page.evaluate(() => window.performance.mark('background-image-frame'));
Expand Down
3 changes: 2 additions & 1 deletion src/plugins/imagery/components/ImageryView.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
@start-pan="startPan"
@toggle-layer-visibility="toggleLayerVisibility"
/>
<div ref="imageBG" class="c-imagery__main-image__bg" @click="expand">
<div ref="imageBG" class="c-imagery__main-image__bg">
<div v-if="zoomFactor > 1" class="c-imagery__hints">
{{ formatImageAltText }}
</div>
Expand All @@ -58,6 +58,7 @@
height: `${sizedImageHeight}px`
}"
@mousedown="handlePanZoomClick"
@dblclick="expand"
>
<div
v-for="(layer, index) in visibleLayers"
Expand Down