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

feat: replace loading message for placeholder #2121

Merged
merged 3 commits into from May 12, 2018

Conversation

Projects
None yet
4 participants
@jeremenichelli
Member

jeremenichelli commented May 7, 2018

Ok, probably unexpected but I hope you like it, specially @skipjack.

What if instead of the Loading... message we have right now:

before-content-loading

We display a more modern placeholder animated like this:

after-2

That last example is serving the actual static + SPA content with CPU and Network throttling, I swear that live it looks really awesome and improves the experience a ton.

I'm also defering related links and contributors until the real markdown content is there.

@@ -10,14 +10,37 @@ import Gitter from '../Gitter/Gitter';
// Load Styling
import './Page.scss';

// Placeholder string

This comment has been minimized.

@EugeneHlushko

EugeneHlushko May 8, 2018

Member

move to placeholder component

This comment has been minimized.

@jeremenichelli

jeremenichelli May 8, 2018

Member

I did that first, but since it just returns a string I thought it would be confusing it's not a React component. I might switch to that.

This comment has been minimized.

@EugeneHlushko

EugeneHlushko May 8, 2018

Member

yeah a component would be perfect imo

@@ -6900,6 +6904,13 @@ react-banner@^1.0.0-rc.0:
version "1.0.0-rc.0"
resolved "https://registry.yarnpkg.com/react-banner/-/react-banner-1.0.0-rc.0.tgz#59627abaf240cdea33b81a9438d499af07aeb103"

react-document-title@^2.0.3:

This comment has been minimized.

@EugeneHlushko

EugeneHlushko May 8, 2018

Member

it seems like a 1st level dependency, where did it come from?

This comment has been minimized.

@jeremenichelli

jeremenichelli May 8, 2018

Member

For some reason after the rebase I'm seeing stuff I didn't modify like this one. I will try to clean this up.

@@ -6957,6 +6968,13 @@ react-router@^4.2.0:
prop-types "^15.5.4"
warning "^3.0.0"

react-side-effect@^1.0.2:

This comment has been minimized.

@EugeneHlushko

EugeneHlushko May 8, 2018

Member

this one too

@EugeneHlushko

This comment has been minimized.

Member

EugeneHlushko commented May 8, 2018

How does "SSR" look, related and contributors still visible in prod build ? If there is no impact here then i think its a good addition

@montogeek

This comment has been minimized.

Member

montogeek commented May 8, 2018

There is no way to make render faster to avoid a placeholder completely? Like the current docs, it takes less than 200ms to load

@jeremenichelli

This comment has been minimized.

Member

jeremenichelli commented May 8, 2018

@montogeek when you are on a wifi network you won't even see the placeholder honestly as it happens too fast. But when your wifi is crappy or you're on a mobile network it really improves the user perception and prevents the content restacking effect.

@EugeneHlushko server side code remains untouch as this only affects Promise based content.

@montogeek

This comment has been minimized.

Member

montogeek commented May 8, 2018

What happens if the user have JS deactivated? I guess old fashion links should work, right?

@montogeek

This comment has been minimized.

Member

montogeek commented May 8, 2018

Once new React API Suspense is released we could improve this to only show it after 150ms.

@jeremenichelli

This comment has been minimized.

Member

jeremenichelli commented May 8, 2018

What happens if the user have JS deactivated? I guess old fashion links should work, right?

I guess so, though that is out of the scope of this PR and it's more on the Rebuild PR plate.

@jeremenichelli jeremenichelli force-pushed the content-placeholder branch from edf17bd to 3bfdf66 May 8, 2018

@skipjack

Love it, please see minor comments (one of which I'm fine with holding off on).

import './Placeholder.scss';

// Placeholder string
const Placeholder = () => (`

This comment has been minimized.

@skipjack

skipjack May 11, 2018

Collaborator

Any reason we're not making this a real react component rather than using a template string?

This comment has been minimized.

@skipjack

skipjack May 11, 2018

Collaborator

Oh I see, because it's passed in dangerouslySetInnerHTML. We could key off the contentLoaded state and use a proper component. Once we use remark-react though we can avoid dangerouslySetInnerHTML entirely and this can definitely become a proper component.

This comment has been minimized.

@jeremenichelli

jeremenichelli May 11, 2018

Member

Yeah, that's the only reason, I actually crafted it initially as a component and had to roll back when I realized that. We can definitely refactor it when we introduce remark to the game.

// Placeholder string
const Placeholder = () => (`
<div class="placeholder">
<p class="placeholder--medium">&nbsp;</p>

This comment has been minimized.

@skipjack

skipjack May 11, 2018

Collaborator

I think these would actually be elements not modifiers so placeholder__medium would make more sense to stay consistent with BEM.

This comment has been minimized.

@jeremenichelli

jeremenichelli May 11, 2018

Member

Yeah totally.

@jeremenichelli

This comment has been minimized.

Member

jeremenichelli commented May 11, 2018

@skipjack I've updated the classnames, do you have any more concerns for now?

@skipjack

This comment has been minimized.

Collaborator

skipjack commented May 12, 2018

Nope, looks great.. merging.

@skipjack skipjack merged commit 97dfa15 into rebuild May 12, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
licence/cla Contributor License Agreement is signed.
Details

@skipjack skipjack deleted the content-placeholder branch May 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment