Skip to content

Commit

Permalink
Fix card list UI layout shifts (jumps) on load
Browse files Browse the repository at this point in the history
This commit fixes layout shifts that occur on card list part of the page
when the page is initially loaded.

- Resolve issue where card list starts with minimal width, leading
  to jumps in UI until correct width is calculated on medium and
  big screens.
- Dispose of existing `ResizeObserver` properly before creating a new
  one. This prevents leaks and incorrect width calculations if
  `containerElement` changes.
- Throttle resize events to minimize width/height calculation changes,
  enhancing performance and reducing the chances for layout shifts.

Supporting CI/CD improvements:

- Enable artifact upload in CI/CD even if E2E tests fail.
- Distinguish uploaded artifacts by operating system for clarity.
  • Loading branch information
undergroundwires committed Nov 16, 2023
1 parent 3864f04 commit bf3426f
Show file tree
Hide file tree
Showing 12 changed files with 425 additions and 63 deletions.
9 changes: 5 additions & 4 deletions .github/workflows/tests.e2e.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ jobs:
-
name: Output artifact directories
id: artifacts
if: always() # Run even if previous steps fail because test run video is always captured
shell: bash
run: |-
declare -r dirs_json_file='cypress-dirs.json'
Expand All @@ -49,15 +50,15 @@ jobs:
echo "VIDEOS_DIR=${VIDEOS_DIR}" >> "${GITHUB_OUTPUT}"
-
name: Upload screenshots
if: failure() # Run only if previous step fails because screenshots will be generated only if E2E test failed
if: failure() # Run only if previous steps fail because screenshots will be generated only if E2E test failed
uses: actions/upload-artifact@v3
with:
name: e2e-screenshots
name: e2e-screenshots-${{ matrix.os }}
path: ${{ steps.artifacts.outputs.SCREENSHOTS_DIR }}
-
name: Upload videos
if: always() # Run even if previous step fails because test run video is always captured
if: always() # Run even if previous steps fail because test run video is always captured
uses: actions/upload-artifact@v3
with:
name: e2e-videos
name: e2e-videos-${{ matrix.os }}
path: ${{ steps.artifacts.outputs.VIDEOS_DIR }}
6 changes: 6 additions & 0 deletions cypress.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ export default defineConfig({
specPattern: `${cypressDirs.base}/**/*.cy.{js,jsx,ts,tsx}`, // Default: cypress/e2e/**/*.cy.{js,jsx,ts,tsx}
supportFile: `${cypressDirs.base}/support/e2e.ts`,
},

/*
Disabling Chrome's web security to allow for faster DOM queries to access DOM earlier than
`cy.get()`. It bypasses the usual same-origin policy constraints
*/
chromeWebSecurity: false,
});

function getApplicationPort(): number {
Expand Down
67 changes: 41 additions & 26 deletions src/presentation/assets/styles/_mixins.scss
Original file line number Diff line number Diff line change
@@ -1,52 +1,67 @@
@mixin hover-or-touch($selector-suffix: '', $selector-prefix: '&') {
@media (hover: hover) {
/* We only do this if hover is truly supported; otherwise the emulator in mobile
@media (hover: hover) {

/* We only do this if hover is truly supported; otherwise the emulator in mobile
keeps hovered style in-place even after touching, making it sticky. */
#{$selector-prefix}:hover #{$selector-suffix} {
@content;
}
#{$selector-prefix}:hover #{$selector-suffix} {
@content;
}
@media (hover: none) {
/* We only do this if hover is not supported,otherwise the desktop behavior is not
}

@media (hover: none) {

/* We only do this if hover is not supported,otherwise the desktop behavior is not
as desired; it does not get activated on hover but only during click/touch. */
#{$selector-prefix}:active #{$selector-suffix} {
@content;
}
#{$selector-prefix}:active #{$selector-suffix} {
@content;
}
}
}

@mixin clickable($cursor: 'pointer') {
cursor: #{$cursor};
user-select: none;
/*
cursor: #{$cursor};
user-select: none;
/*
It removes (blue) background during touch as seen in mobile webkit browsers (Chrome, Safari, Edge).
The default behavior is that any element (or containing element) that has cursor:pointer
explicitly set and is clicked will flash blue momentarily.
Removing it could have accessibility issue since that hides an interactive cue. But as we still provide
response to user actions through :active by `hover-or-touch` mixin.
*/
-webkit-tap-highlight-color: transparent;
-webkit-tap-highlight-color: transparent;
}

@mixin fade-transition($name) {
.#{$name}-enter-active,
.#{$name}-leave-active {
transition: opacity 0.3s ease;
}

.#{$name}-enter-from,
.#{$name}-leave-to {
opacity: 0;
}
}

@mixin fade-slide-transition($name, $duration, $offset-upward: null) {
.#{$name}-enter-active,
.#{$name}-leave-active {
transition: all $duration;
}

.#{$name}-leave-active,
.#{$name}-enter-from
{
opacity: 0;
.#{$name}-enter-active,
.#{$name}-leave-active {
transition: all $duration;
}

@if $offset-upward {
transform: translateY($offset-upward);
}
.#{$name}-leave-active,
.#{$name}-enter-from {
opacity: 0;

@if $offset-upward {
transform: translateY($offset-upward);
}
}
}

@mixin reset-ul {
margin: 0;
padding: 0;
list-style: none;
}
}
63 changes: 34 additions & 29 deletions src/presentation/components/Scripts/View/Cards/CardList.vue
Original file line number Diff line number Diff line change
@@ -1,34 +1,36 @@
<template>
<SizeObserver v-on:widthChanged="width = $event">
<!--
<div id="responsivity-debug">
Width: {{ width || 'undefined' }}
Size:
<span v-if="width <= 500">small</span>
<span v-if="width > 500 && width < 750">medium</span>
<span v-if="width >= 750">big</span>
<transition name="fade-transition">
<div v-if="width">
<!-- <div id="responsivity-debug">
Width: {{ width || 'undefined' }}
Size:
<span v-if="width <= 500">small</span>
<span v-if="width > 500 && width < 750">medium</span>
<span v-if="width >= 750">big</span>
</div> -->
<div
v-if="categoryIds.length > 0"
class="cards"
>
<CardListItem
class="card"
v-bind:class="{
'small-screen': width <= 500,
'medium-screen': width > 500 && width < 750,
'big-screen': width >= 750,
}"
v-for="categoryId of categoryIds"
:data-category="categoryId"
v-bind:key="categoryId"
:categoryId="categoryId"
:activeCategoryId="activeCategoryId"
v-on:cardExpansionChanged="onSelected(categoryId, $event)"
/>
</div>
<div v-else class="error">Something went bad 😢</div>
</div>
-->
<div
v-if="categoryIds.length > 0"
class="cards"
>
<CardListItem
class="card"
v-bind:class="{
'small-screen': width <= 500,
'medium-screen': width > 500 && width < 750,
'big-screen': width >= 750,
}"
v-for="categoryId of categoryIds"
:data-category="categoryId"
v-bind:key="categoryId"
:categoryId="categoryId"
:activeCategoryId="activeCategoryId"
v-on:cardExpansionChanged="onSelected(categoryId, $event)"
/>
</div>
<div v-else class="error">Something went bad 😢</div>
</transition>
</SizeObserver>
</template>

Expand All @@ -49,7 +51,8 @@ export default defineComponent({
setup() {
const { currentState, onStateChange } = injectKey((keys) => keys.useCollectionState);
const width = ref<number>(0);
const width = ref<number | undefined>();
const categoryIds = computed<readonly number[]>(
() => currentState.value.collection.actions.map((category) => category.id),
);
Expand Down Expand Up @@ -138,4 +141,6 @@ function isClickable(element: Element) {
font-size: 3.5em;
font-family: $font-normal;
}
@include fade-transition('fade-transition');
</style>
6 changes: 4 additions & 2 deletions src/presentation/components/Shared/SizeObserver.vue
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
defineComponent, shallowRef, onMounted, onBeforeUnmount, watch,
} from 'vue';
import { useResizeObserverPolyfill } from '@/presentation/components/Shared/Hooks/UseResizeObserverPolyfill';
import { throttle } from '@/presentation/components/Shared/Throttle';
export default defineComponent({
emits: {
Expand All @@ -34,10 +35,11 @@ export default defineComponent({
return;
}
resizeObserverReady.then(() => {
observer = new ResizeObserver(updateSize);
disposeObserver();
observer = new ResizeObserver(throttle(updateSize, 200));
observer.observe(element);
});
updateSize();
updateSize(); // Do not throttle, immediately inform new width
}, { immediate: true });
});
Expand Down

0 comments on commit bf3426f

Please sign in to comment.