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

MNTOR-971: Landing page section transitions #2983

Merged
merged 14 commits into from Apr 17, 2023

Conversation

flozia
Copy link
Collaborator

@flozia flozia commented Apr 12, 2023

References:

Jira: MNTOR-971
Design spec: https://ff-landing-page.webflow.io/#color

Description

Adds simple appearance transitions to sections on the landing page.

Screenshot

mntor-971-screenrec.mov

How to test

  1. Navigate to the landing page: /
  2. Scroll through the landing page

Checklist (Definition of Done)

  • Localization strings (if needed) have been added.
  • Commits in this PR are minimal and have descriptive commit messages.
  • I've added or updated the relevant sections in readme and/or code comments
  • I've added a unit test to test for potential regressions of this bug.
  • Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
  • All acceptance criteria are met.
  • Jira ticket has been updated (if needed) to match changes made during the development process.
  • Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

@flozia flozia requested review from Vinnl and toufali April 13, 2023 13:21
src/client/js/section-observer.js Outdated Show resolved Hide resolved
src/client/js/section-observer.js Outdated Show resolved Hide resolved
src/client/js/section-observer.js Outdated Show resolved Hide resolved

const classNameToObserve = 'section-transition'
const classNameEntered = `${classNameToObserve}-entered`
const cueIntervalDuration = 150
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That’s a bit shorter than what is shown in the design prototype but feels more responsive. Alignment on this with UX is TBD.

Copy link
Member

Choose a reason for hiding this comment

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

+1 on speedy transitions! 👍

Copy link
Collaborator

@Vinnl Vinnl left a comment

Choose a reason for hiding this comment

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

Have you considered leaving most of your inline comments in this PR as code comments? 😁

Other than that, one thing that might be annoying is that, if the JS doesn't load, or loads slowly (or is blocked, or broken), content won't be visible by default. But on the other hand, if we let JS hide the content initially, that could result in a quick flash of content before it animates in, which also isn't great. So I guess I don't have a better solution there 🤷

src/client/js/section-observer.js Outdated Show resolved Hide resolved
src/client/js/section-observer.js Outdated Show resolved Hide resolved
@flozia
Copy link
Collaborator Author

flozia commented Apr 13, 2023

Have you considered leaving most of your inline comments in this PR as code comments? 😁

I did but now I’ve added them as well: fb036ad. :)

Other than that, one thing that might be annoying is that, if the JS doesn't load, or loads slowly (or is blocked, or broken), content won't be visible by default. But on the other hand, if we let JS hide the content initially, that could result in a quick flash of content before it animates in, which also isn't great. So I guess I don't have a better solution there 🤷

Good point @Vinnl. I’m not really happy about this only working with JS as well. The flashing you describe would somewhat defeat the purpose of an appearance transition and in that case, I’d prefer no transition at all. Looking forward to animation-timeline evolving out of experimental mode! We could at least take care of instances where JS is not allowed — what do you think about this approach: e774f14?

Copy link
Member

@toufali toufali 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 looking good @flozia!

It works for me and I had only one "blocker-ish" thought -- I wonder if we should remove the transition from the Hero section? I don't know if it impacts CLS, but the hero does have a button and forthcoming form. Perhaps more a UX decision, but I wonder if it would be best to only apply the fly-in effect to sections that are just coming into view – not those already in view? Or at least not the very first element?

Additional thoughts more about implementation – no blockers just potential simplifiers/optimizers:

  • I notice you're tracking when a section is entered/transition has been run. Have you already tried using animation instead of transition? Using animation is often well-suited for run-once type behavior.
  • For the no-script users, animation might also benefit – it works using only a "to" property, which can keep sections visible unless JS is available to inject the proper initial state. Pseudo-ish code:

CSS:

[data-animate] {
  /* available by default to all with no-script defaults */
  opacity: var(--section-animation-opacity, 1);
  transform: translateY(var(--section-animation-y, 0));
}

[data-animate='visible'] {
  /* triggered via script when element in viewport */
  animation: section-animation 1s forwards;
}

@keyframes section-animation {
  to {
    opacity: 1;
  }
}

JS:

function init(){
  // we know script is available so we can populate initial state into vars
  // we can add these once in top level in `<html>` and it will apply to all associated elements
  document.documentElement.style.setProperty('--section-animation-opacity', '0')
  document.documentElement.style.setProperty('--section-animation-y', '24px')
}

function handleInteresectionEvent(){
  if (entry.isIntersecting) {
    element.dataset.animate = 'visible'
  }
}

HTML:

<section data-animate>...</section>

Happy to chat further or pair on this if you like!

Comment on lines 8 to 14
<noscript>
<style>
.section-transition {
opacity: 1;
}
</style>
</noscript>
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there's another way we can handle no-script users, than having to add this snippet on every page with transitions? See thoughts in comment above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This works; considering animation sounds good too. It doesn't necessarily deal with slow-loading JS; we could potentially also add a delayed animation in CSS that changes the opacity to 1 after a certain delay, and if the JS loads in time it can disable that and replace it with the actual animation trigger. But also, let's maybe not overthink it, as it's probably pretty much an edge case.

Copy link
Collaborator Author

@flozia flozia Apr 14, 2023

Choose a reason for hiding this comment

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

I’ve added a more generic way to add entering transitions for the site. Switched to CSS variables as suggested: c76e41d.

Copy link
Collaborator Author

@flozia flozia Apr 14, 2023

Choose a reason for hiding this comment

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

… and 9453676 should take care of JS that is loading too slow or not at all.

* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

const classNameToObserve = 'section-transition'
const classNameEntered = `${classNameToObserve}-entered`
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is to track if a transition has run, and that it runs only once? Perhaps we could avoid this overhead by using animation instead of transition?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need a way to check which sections we need to add to the queue. Checking for the data attribute enter-transition now.

Comment on lines 241 to 250
.section-transition {
opacity: 0;
transition: opacity 350ms ease-out, transform 350ms ease-out;
transform: translateY(var(--padding-lg));
}

.section-transition-entered {
opacity: 1;
transform: translateY(0);
}
Copy link
Member

Choose a reason for hiding this comment

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

Have you already explored using animation? Brainstorming here, but it might simplify and solve the cases of no-script and re-triggering transitions. See comment above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea! I’m using animations instead of transitions now.

}

if (!observers) {
const mediaQuery = window.matchMedia('(prefers-reduced-motion: no-preference)')
Copy link
Member

Choose a reason for hiding this comment

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

Is this really necessary if we've already got @media screen and (prefers-reduced-motion: no-preference) in the CSS? Or maybe the CSS at-rule is not necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See comment below: #2983 (comment).

src/client/js/section-observer.js Outdated Show resolved Hide resolved
@flozia flozia requested a review from toufali April 14, 2023 12:15
@flozia
Copy link
Collaborator Author

flozia commented Apr 14, 2023

Thanks for the suggestions @toufali! I switched to using animations and I am using data-attributes as well as CSS variables instead.

Good point about potential issues we run into with hiding the hero section — I’ll raise that with UX.

@flozia flozia marked this pull request as ready for review April 14, 2023 12:23
@flozia flozia merged commit e8dceb9 into main Apr 17, 2023
9 checks passed
@flozia flozia deleted the MNTOR-971-Landing-page-transitions branch April 17, 2023 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants