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

React router transitions #792

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

React router transitions #792

wants to merge 29 commits into from

Conversation

nzambello
Copy link
Member

@nzambello nzambello commented Jun 22, 2019

Fixes #654.

I renamed ScrollToTop to AnimationWrapper because I added functionalities to it.
Now, when it detect a location change, applies some CSS classes. With those CSS classes, we can easily animate on opacity or whatever else and it's easy to change animations unlike with js libraries.

@nzambello nzambello self-assigned this Jun 22, 2019
@nzambello nzambello force-pushed the react-router-transitions branch from 6d8f2c6 to 1c8be90 Compare Jun 22, 2019
@robgietema
Copy link
Member

@robgietema robgietema commented Jun 23, 2019

When I test the branch I see the header is a bit strange, the logo is above the navbar and the searchbar below. Some weird side effects. Any ideas?

@nzambello
Copy link
Member Author

@nzambello nzambello commented Jun 24, 2019

@robgietema I saw it while developing, but not in the last version I committed.
Anyway, the only thing I can think about is the div I have put for handling css classes, but it's not in the header. 🤔

@sneridagh sneridagh added this to the 4.0.0 milestone Aug 19, 2019
@tisto tisto added this to In progress in Volto 4 Final Sep 20, 2019
@tisto
Copy link
Member

@tisto tisto commented Sep 22, 2019

@nzambello the router animations work like a charm. I'd really like to merge this now so we can try it out in our projects and find possible issues. Though, the header is currently somehow broken:

Events

Could you image to have a look?

@tisto
Copy link
Member

@tisto tisto commented Sep 22, 2019

FYI: There are also two accessibility violations on the front page that are not in master.

@tisto
Copy link
Member

@tisto tisto commented Sep 22, 2019

Seems this has something to do with Server Side Rendering. This error pops up in the console:

Warning: Expected server HTML to contain a matching <div> in <div>.

Copy link
Member

@tisto tisto left a comment

The animations itself work like a charm. We have a problem with the hydrate function. It seems the HTML from the SSR differs from the client-side HTML.

Volto 4 Final automation moved this from In progress to Review in progress Sep 29, 2019
sneridagh added 5 commits Dec 10, 2019
* master: (116 commits)
  Remove "documentDescription" class in table block (#1015)
  Added CTRL+ENTER feature in text blocks by default. It creates a newline inside the same text chunk (`<p>`) (#1044)
  Revert "Fix make build on py3."
  Add plone.rest(api) to sources.
  Fix make build on py3.
  Fix lint check on bash condition (#1031)
  Further upgrade documentation
  Back to development
  Release 4.0.0-alpha.17
  Add docs for upgrade to alpha 17
  Reverteslintupgrade (#1028)
  Back to development
  Release 4.0.0-alpha.16
  Lodash improvements for decrease bundle size (#975)
  Fix small CSS issues in Blocks (#1023)
  Forked `react-helmet` since it seems unmaintained. Now it's a Named import in helpers (#1025)
  Solvepeerdependencies (#1026)
  Fix to a good know digest (#1022)
  Back to development
  Release 4.0.0-alpha.15
  ...
Copy link
Member

@sneridagh sneridagh left a comment

Fixed the sync between client and server, and the CSS associated with the main container.

@sneridagh
Copy link
Member

@sneridagh sneridagh commented Dec 10, 2019

@tisto @robgietema this one is ready, maybe we have to check if the animation is the right one that we want.

@sneridagh sneridagh moved this from Review in progress to Reviewer approved in Volto 4 Final Dec 10, 2019
@AlexBueckig
Copy link
Member

@AlexBueckig AlexBueckig commented Dec 10, 2019

After talking to Timo Till and I had a look at the opacity values because a value of 0.3 seems to be a bit rough. We played around with the values a little bit and here's what we thought works best for the transition (line 266 - 276 in main.less):

  .content-area {
    position: relative;
    opacity: 0.7;
    transition: opacity 0.1s;
  }

  &.entering {
    .content-area {
      opacity: 0.7;
    }
  }

@sneridagh sneridagh removed this from Reviewer approved in Volto 4 Final Jan 25, 2020
@nzambello
Copy link
Member Author

@nzambello nzambello commented Feb 14, 2020

@sneridagh how about this? what's missing? I didn't follow the updates so well, but I tested out right now and I see this ready and it works fine.

@nzambello nzambello requested review from tisto and sneridagh Mar 10, 2020
@nzambello
Copy link
Member Author

@nzambello nzambello commented Mar 10, 2020

@tisto @sneridagh
Plone tests are failing but locally I'm not able to reproduce the error, test work fine and pass.
Furthermore I'm really interested in pushing this to a near release, so let me know what you think about this.

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.

6 participants