Replace the current spinner with Calypso's new pure CSS spinner.

[kwight]

This PR replaces the original poorly-performing spinner with a simpler CSS spinner, which was recently updated in Calypso.

Fixes #234 .

Testing

• Change both the isLoading prop here and the isSubmitting prop here to true, so you can see what's going on.
• Load notifications, and verify the note list spinner is correct.
• Reply to a comment, and verify the reply spinner looks correct.

[kwight]

[kwight]

This also tidies up https://github.com/Automattic/notifications-panel/issues/110#issuecomment-357057062.

[folletto]

This also tidies up #110 (comment).

You mean it also fix the constant resetting or just the cropping?

[kwight]

@folletto sorry, just the cropping; the resetting I imagine to be an issue around constant re-renders, which I'll need to dig into separately.

[folletto]

Understood! Thank you! :)

[gwwar]

Looks like the parent is being rendered multiple times here which is resetting the animation. Note that setting the following doesn't work https://developer.mozilla.org/en-US/docs/Web/CSS/animation-fill-mode

Possibly from polling? Looks like existing behavior so non-blocking but nice to tidy.

[gwwar]

🤷‍♀️ Did I never notice before that our polling behavior grabs 10, then 20, ... up to 100 notes for polling?

As far as I can tell it's starting from the same note range. Eg note 0 is the same for the first request as it is for the 10th one.

Ran out of time, but likely this is flickering from us dispatching actions after each polling request comes in.

[dmsnell]

Did I never notice before that our polling behavior grabs 10, then 20, ... up to 100 notes for polling?

This is the last major bit of unfinished business I have on my todo list for the notes client. It's not trivial to adjust. First of all, the reason we always grab the newest notes is because they might be changing and we wouldn't want to miss a new update because we're grabbing the old ones.

The thing to realize is that the app state is a nightmare. I don't think it would take another full week's meetup to change but maybe a week of one person dedicated to the cause.

[kwight]

For the rendering issue, let's tackle it in a separate PR addressing #110 .

Out of curiosity is there a reason why we don't namespace all of our classes? Or did we feel the parent class was enough?

Since we're using Calypso's spinner, my thought was towards keeping the code the same between the two repos (hence the // start and // end comments); otherwise, any changes to Calypso's styles and markup mean updating the style prefix every time when porting those changes to this repo (although, I'm the first to admit this isn't a lot of unreasonable work). Strong feelings either way?

Let's have wpnc__spinner be a default class

Not sure what you mean here?

[vindl]

Let's have wpnc__spinner be a default class

Not sure what you mean here?

My guess would be to move it to component render method instead of passing it as a prop. https://github.com/Automattic/notifications-panel/pull/236/files#diff-a555cad79b68b4f00c9b34c45ac49947R359

[kwight]

OK, I prefixed classes and set wpnc__spinner as the default class, we should be ready to go.

[vindl]

Since we're using Calypso's spinner, my thought was towards keeping the code the same between the two repos (hence the // start and // end comments); otherwise, any changes to Calypso's styles and markup mean updating the style prefix every time when porting those changes to this repo (although, I'm the first to admit this isn't a lot of unreasonable work). Strong feelings either way?

Since we are using Sass could we extend from existing class, so we can have it namespaced and avoid duplication at the same time? (I'm guessing that the answer is no because of separate codebases, and all the different places that notes are used in)

.wpnc__spinner {
    @extend .spinner;
    ...
}

If not, then I think that adding a small maintenance burden by having everything prefixed consistently is fine. Should we also add the comments in Calypso counterpart in order to inform future devs to contact us for syncing if they decide to change the relevant parts of the spinner?

[folletto]

I'm guessing that the answer is no because of separate codebases, and all the different places that notes are used in

That would be the reason for my "no" too. I feel it's better to have a risk to have a component outdated than the risk of breaking it entirely for a missing reference that isn't automatically enforced.

Should we also add the comments in Calypso counterpart in order to inform future devs to contact us for syncing if they decide to change the relevant parts of the spinner?

This sounds like a good idea! Like "Open a PR also on notifications-panel if the component gets updated" kind of thing?

[gwwar]

Since we are using Sass could we extend from existing class

Let's avoid extend, overall I find it hard to maintain (media queries don't work) and it's very easy to add css selector bloat accidentaly. If we want to use the spinner as a generic component, we can pull it out of Calypso entirely and use it as an npm dependency.

Let's 🚢 this for now, and see if we need to pull out that component as a dependency later. I don't think the spinner will be worked on that often, so we may not need to do htis.

[vindl]

This sounds like a good idea! Like "Open a PR also on notifications-panel if the component gets updated" kind of thing?

Yes, that's exactly what I had in mind. :+1:

[kwight]

Instructions added to Calypso in https://github.com/Automattic/wp-calypso/pull/23145.