EHN Implement the new logo on the loading animation #1546
Conversation
|
Is this to also be used in place of the ss-loading-screen animation? Can you please also add a video or gif of it working so we can be sure what you're intending is what we've ended up with? |
| <div key="overlay" className="cms-content-loading-overlay ui-widget-overlay-light" /> | ||
| <div key="spinner" className="cms-content-loading-spinner"> | ||
| <div className="spinner"> | ||
| {/* window.location is neccessary for Safari which otherwise prepends the base tag */} |
There was a problem hiding this comment.
I've decided to throw Safari under the bus here. Basically, forcing us to use absolute URL to find those SVGs introduce a bug where sometime if you've updated the URL with react route or added some extra parameter, the SVG path clipping doesn't work.
Using the IDs directly instead of the full URL seems to mitigate this problem.
Basically, I think that having this work consistently in all other browser is better than adding a hack so that Safari users get it looking at it's best some of the time.
Safari user should still get a reasonable animation in most cases. It just won't have the nice curvy bit. Given that those loading animation are only shown for a fleeting second and are pretty small, I doubt anyone will even notice.
There was a problem hiding this comment.
Safari will probably look something like this.
| @@ -0,0 +1,111 @@ | |||
| $dash: 750; | |||
There was a problem hiding this comment.
I've moved this bit from _style.scss so it lives next to the actual component its meant to style.
| @@ -1011,6 +1007,14 @@ <h2>CSS mapping</h2> | |||
| <div class="icon font-icon-attention-1"></div> | |||
| <input type="text" readonly="readonly" value="attention-1"> | |||
| </li> | |||
| <li> | |||
| <div class="icon font-icon-silverstripe"></div> | |||
There was a problem hiding this comment.
I've kept the old Siverstripe icon in there, in case we decide we have a use case for showing the company logo.
client/src/images/spinner.gif
Outdated
There was a problem hiding this comment.
This is reference in one SCSS file for some jQuery modal. I don't actually think this is used anywhere. We seem to have a lot of dead code in our CMS UI.
I've updated it anyway, but I think we should have a follow up card to remove dead weight code.
| &__logo { | ||
| &__part { | ||
| fill: $blue; | ||
| stroke: $blue; | ||
| stroke-width: 1; | ||
| stroke-linejoin: miter; | ||
| stroke-dasharray: 225; | ||
| stroke-dashoffset: 0; | ||
|
|
||
| animation: logo__draw 2s cubic-bezier(.445, .05, .55, .95) 2 alternate both 1s; | ||
|
|
||
| @keyframes logo__draw { | ||
| 0% { | ||
| fill-opacity: 1; | ||
| stroke-dashoffset: 0; | ||
| } | ||
| 40% { | ||
| fill-opacity: 0; | ||
| stroke-dashoffset: 0; | ||
| } | ||
| 90% { | ||
| fill-opacity: 0; | ||
| stroke-dashoffset: 225; | ||
| } | ||
| 100% { | ||
| fill-opacity: 0; | ||
| stroke-dashoffset: 225; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This block was only there for the old animation
There was a problem hiding this comment.
This pure dead weight code.
I've removed all the matching images.
maxime-rainville
force-pushed
the
pulls/2/loading-animation
branch
from
66483d8
to
21ac658
Compare
|
Here's what this thing looks like. The video looks a bit skipy, but when you look at live it's very smooth. Screencast.from.17-08-23.19.17.33.webmI've validated that looks the same in Chrome. |
There was a problem hiding this comment.
There's stuff changing in this PR for which there's no explanation:
- silverstripe.ttf added / silverstripe.eot added / silverstripe.woff deleted - I'm not sure why this is happening - based on https://medium.com/@aitareydesign/understanding-of-font-formats-ttf-otf-woff-eot-svg-e55e00a1ef2 seems that .eot is for old versions of IE?
- Deleting BrowserWarning and associated files - this seems like random refactoring which I really don't like in PRs as it clutters their intent - this should have been done in a separate PR / issue
Please explain the first point
Also get this design reviewed. I tested on my local and honestly this looked pretty bad IMO in firefox. Experience was a flickering icon that had a solid white background (possibly it's supposed to be transparent?). Particularly bad was switching between model admins where a large icon flickered followed b a smaller icon flickering.
https://youtu.be/R1nMfWP-ze8 < flickering icon in firefox
I think maybe it would be better to just get rid of the 'loading' icon that pops up in the middle of the CMS altogether?
I'd be against this, as it appears for a scenario where javascript isn't available (see screenshot) and likely in a variety of other similar scenarios that aren't typical, but indicate something's gone wrong. It's not a massively common use case so if people feel strongly about removing it then I won't kick up a fuss, but just an extra scenario to consider. It may also be seen for longer periods of time for users with slow internet connections or scenarios where the CMS takes longer-than-usual to load, which is probably the stronger argument for keeping it. |
Those fonts are managed via Fontastic. We get all those fonts provided to us in a ZIP file. Our process when updating the font has always been to take the entire package and copy it to our code base so we have the full reference.
There's a bunch of logos that are only used in the browser warning. The browser warning is not in-line with our minimum browser requirements. In this context, it would be a waste of time to update logos for a warning that is not relevant any more.
James will be looking at this once the code review is done.
The only thing that has changed here is the logo. To the extent there's flickering it would have been there before. Also, keep in mind you are running this thing locally with zero latency. That's not going to be a typical example. |
|
OK code is fine, will leave the 'changes requested on' on for now until the design review so it's not accidentally merged before that's been done |
There was a problem hiding this comment.
Here's the feedback from James and my reply.
Looks great! There’s just a couple of things to tweak if possible…
“Site title in the top left when the side bar is collapse” - can this be white? We’re no longer using the cyan colourI'll change it to white.
Everytime I load a page I get the double loading indicator - a flash of the big one and then the small one. Is it supposed to be showing both?
That was annoying Steve as well.
That's basically how the CMS admin UI is built right now. Addressing this would go way beyond updating the logo.
We could use the little logo everywhere or tweak the look of the indicator, but there's not really a short term way we could get rid of the double loading indicator.display: none on the big one?? lol
We could do something like transitioning it to opacity 0
videoThat’s definitely better. It was more about when I saw the big one flick on for a micro-second and then seeing the smaller one.
So basically. I change the logo in the top left to white and I added a fadeout to the big logo loading screen.
| @@ -11,7 +11,7 @@ $brand-success: #3fa142; | |||
| $brand-danger: #d40404; | |||
|
|
|||
| // Offical SilverStripe brand color | |||
| $color-brand: #43c7f4 !default; | |||
| $color-brand: white !default; | |||
There was a problem hiding this comment.
The title logo is the only place where this logo colour is used.
client/src/styles/legacy/_style.scss
Outdated
| opacity: 0; | ||
| transition: opacity .5s ease-in-out; | ||
|
|
||
| &__logo { | ||
| &__part { | ||
| fill: $blue; | ||
| stroke: $blue; | ||
| stroke-width: 1; | ||
| stroke-linejoin: miter; | ||
| stroke-dasharray: 225; | ||
| stroke-dashoffset: 0; | ||
|
|
||
| animation: logo__draw 2s cubic-bezier(.445, .05, .55, .95) 2 alternate both 1s; | ||
|
|
||
| @keyframes logo__draw { | ||
| 0% { | ||
| fill-opacity: 1; | ||
| stroke-dashoffset: 0; | ||
| } | ||
| 40% { | ||
| fill-opacity: 0; | ||
| stroke-dashoffset: 0; | ||
| } | ||
| 90% { | ||
| fill-opacity: 0; | ||
| stroke-dashoffset: 225; | ||
| } | ||
| 100% { | ||
| fill-opacity: 0; | ||
| stroke-dashoffset: 225; | ||
| } | ||
| } | ||
| } | ||
| body.loading & { | ||
| opacity: 1; |
There was a problem hiding this comment.
This snippet adds an opacity transition when the loading class is removed from the <body>.
There was a problem hiding this comment.
While it certainly looks better, the fade that's been added this is a very signficiant UI downgrade so I cannot approve this in its current state
The issue is that the fade adds a significant lag to the responsiveness to the cms because you need to wait for the fade in animation, which applies to the ENTIRE cms rather than just the logo, before you can click anything
I've uploaded a video https://www.youtube.com/watch?v=CAKDL6qKE6o that shows this - bugfix1.test is the existing state where I can quickly click between the models admins. bugfix11.test is with PR and shows how much slower it is
Perhaps if you make it so the fade affect is applied specifically to the logo and not the entire CMS we'll end up with a good result?
|
The logo is in a ligthbox. Applying the fade to the logo only wouldn't do anything because we would still need to keep the lightbox there and it would block you from interacting with whatever is beneath it. On a normal internet connection, you won't be able to interact with the UI anyway because it will still be in the process of fetching things like site trees or Page forms. I'll go back to James and see if he's got extra feedback. If this is controversial, I'd say let's preserve the status quo and take out the fade. |
That's not quite true. On a slow internet connect yes things do take longer to load. But then after they've loaded you THEN still need to wait for the fade animation before you can click anything.
Agree. While I don't like the flickering it is the current behaviour. One thing that could mitigate the flickering is to have a transparent rather than white background for the large logo which should reduce how noticeable it is. The existing large logo has a transparent background |
maxime-rainville
force-pushed
the
pulls/2/loading-animation
branch
from
3147336
to
a77f54b
Compare
|
I removed the loading animation fade and created a follow up card. #1557 |
Parent issue