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

Notices: Add to logged out layout #23951

Merged
merged 3 commits into from
Apr 9, 2018
Merged

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Apr 6, 2018

Add GlobalNotices to the logged-out layout. This allows notices to be displayed on logged-out pages.

Remove GlobalNotices from the login component to avoid duplication when viewing login while… logged in. (https://github.com/Automattic/wp-calypso/pull/13700/files#r179679601)

Add relevant state mock for renderToString layout tests.

Background

I was looking for evidence that GlobalNotices was intentionally removed or omitted from the logged-out layout but could not find any. Here's what I was able to uncover:

  1. Many moons commits ago, there were logged-in and logged-out layouts that both included GlobalNotices:

    <GlobalNotices id="notices" notices={ notices.list } forcePinned={ 'post' === this.props.section } />

    <GlobalNotices id="notices" notices={ notices.list } />

  2. A design-specific logged-out layout also existed, it's creation was described in part as:

    It also ES6ifies the other logged-out layouts, from which it is copied (and slightly modified). This is meant to fix Themes: Render a separate logged-out layout for /design #1325.

    This omitted GlobalNotices. Was that intentional? (cc: @ockham @ehg)

  3. The logged-out layout was removed, with the exception of a design-specific version (Layout: Replace LayoutLoggedOut and LayoutLoggedOutOAuth with Layout #2964)

  4. The design-specific version became the generic logged-out layout (Themes: Render logged-out layout last #3516). This maintains the omission of GlobalNotices, which continues in the logged-out layout that we know and love today.

It appears the GlobalNotices was likely lost in the shuffle. It may have been deemed unnecessary for the design layout, then overlooked when it became the generic logged-out layout.

After all this, the login view needed to show notices (#13700).

It also appears that notices will be needed in other logged-out views (#23846).

Testing

  1. Smoke test
  2. Try dispatching notices from logged out views:
    dispatch({"type":"NOTICE_CREATE","notice":{"showDismiss":true,"status":"is-error","text":"Danger, Will Robinson!"}})
  3. Visit /log-in while logged in, you should see exactly one instance of the NoticesList component in the React tree.
  4. Login: Make it possible to send 2FA recovery code via sms #13700 appears to use local notices now. These instructions appear invalid.
    Try notice-related instructions from Login: Make it possible to send 2FA recovery code via sms #13700

    Assert that an error notice is displayed when clicking either recovery code link twice in under a minute.

@matticbot
Copy link
Contributor

@sirreal sirreal self-assigned this Apr 6, 2018
@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs e2e Testing and removed [Status] In Progress labels Apr 6, 2018
@sirreal sirreal requested review from Tug, yurynix, stephanethomas and a team April 6, 2018 10:20
Copy link
Contributor

@seear seear left a comment

Choose a reason for hiding this comment

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

Tests well for me. Tried some other logged-out pages: /start, /jetpack/connect, /themes. All work fine and display notices on dispatch.

Can also see the component mounting on server render:

  calypso:server-render cache access for key +300ms /log-in
  calypso:server-render cache miss for key +0ms /log-in
  calypso:notices Mounting Global Notices React component. +14ms
  calypso:server-render Server render time (ms) +58ms 72
GET /log-in 200 210.226 ms - 38659

👍

@yurynix
Copy link
Contributor

yurynix commented Apr 8, 2018

My first thought when seeing that was that maybe it was omitted to reduce logged out bundle size, but it seems there is no (significant) effect:
http://iscalypsofastyet.com/branch?branch=add/logged-out-layout-notices

Commit: 2bc157b008a1340de10ed127465afa59c71ec410
Author: sirreal
At: 2018-04-06T09:18:03Z
Message: Add notices state mock to render test
Delta:
chunk     stat_size           parsed_size           gzip_size
build        +230 B  (+0.0%)       +168 B  (+0.0%)      +56 B  (+0.0%)
login        -169 B  (-0.1%)        -56 B  (-0.1%)       -4 B  (-0.0%)
manifest       +0 B                  +0 B                +2 B  (+0.1%)

@sirreal sirreal added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs e2e Testing labels Apr 9, 2018
@sirreal sirreal merged commit 69c1b25 into master Apr 9, 2018
@sirreal sirreal deleted the add/logged-out-layout-notices branch April 9, 2018 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Themes: Render a separate logged-out layout for /design
4 participants