Skip to content
This repository has been archived by the owner on Aug 26, 2021. It is now read-only.

Add loading overlay to chameleon iframe while template is being generated #375

Merged
merged 23 commits into from
Jan 22, 2019

Conversation

sukhrajghuman
Copy link
Contributor

@sukhrajghuman sukhrajghuman commented Jan 9, 2019

what do you think? could have a toast notification instead

@sukhrajghuman sukhrajghuman changed the base branch from master to develop January 9, 2019 00:33
@sukhrajghuman sukhrajghuman self-assigned this Jan 9, 2019
@sukhrajghuman sukhrajghuman changed the title Add loading overlay to chameleon iframe when template is being generated Add loading overlay to chameleon iframe while template is being generated Jan 9, 2019
Copy link
Contributor

@alex-page alex-page left a comment

Choose a reason for hiding this comment

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

@sukhrajghuman this is great work. Some of the things I have noticed:

  • The spinner disappears then after a delay the template changes the colour. The spinner should show until the colour is changed.
  • The delay was changed from 400 -> 1000. I don't think this was intentional. Remember to have a quick look at the files changed on GitHub.
  • role="alert" would be a frustrating user experience as every time it gets shown the alert would get read. I personally would hate hearing "Loading your template" every time a new request is sent to Chameleon.
  • background-color: #ffffff63; this should use rgba, also you should test this in IE8.
  • You are using background and background-color you can probably just use background, or change the other css to specific properties
  • When you use AU-svguri you don't need classes, width or height. Have a look at some of the other implementations of icons in the _icons.scss file.
  • The SVG itself is quite large, look at how we have split icons into multiple lines
  • Maybe a different SVG or animated GIF would be more appropriate in this situation as there are quite a lot of parts to this one.

@sukhrajghuman
Copy link
Contributor Author

sukhrajghuman commented Jan 15, 2019

thanks for feedback @alex-page

  • My bad i forgot to remove the role='alert'
  • Yeah i need to work out how to make the transition from the loader more smooth

background-color: #ffffff63; this should use rgba, also you should test this in IE8.

Nice point

When you use AU-svguri you don't need classes, width or height. Have a look at some of the other implementations of icons in the _icons.scss file.

Yep the svg was quite large, I think a gif will be probs be better

You are using background and background-color you can probably just use background, or change the other css to specific properties

For some reason when I put them as one, the overlay didn't show up. Will investiagte it further.

@alex-page
Copy link
Contributor

Nice happy to help mate!

@sukhrajghuman
Copy link
Contributor Author

sukhrajghuman commented Jan 17, 2019

When you use AU-svguri you don't need classes, width or height. Have a look at some of the other implementations of icons in the _icons.scss file.

Yes that's correct. This is not an icon though. The default height and width for our icons is 1.25em. That would be too small. Is that what you meant?

@alex-page
Copy link
Contributor

alex-page commented Jan 22, 2019

@sukhrajghuman nice work. I have changed a few things:

  • Moved the animation to CSS keyframes with rotate. window.push.state works in IE10 so I think we should support the animation in IE10 as well. Unfortunately animateTransform is not supported in IE10.
  • Moved the icon to an pseduo element so we can rotate the svg without the background
  • Changed when the loading spinner is hidden, previously it used setTimeout and now it uses an onload event on the iframe
  • Adjusted the width and height of the SVG slightly
  • Moved the event listener from keyup to input
  • Moved the width and height of the SVG to css

Copy link
Contributor

@azerella azerella left a comment

Choose a reason for hiding this comment

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

With the new input event listener changes, LGTM 👍

Feels responsive, flud and all around just a great feeling

@alex-page
Copy link
Contributor

@sukhrajghuman let me know if you are happy with the above changes.

@sukhrajghuman
Copy link
Contributor Author

nice mate much better

@sukhrajghuman sukhrajghuman merged commit a20ee9a into develop Jan 22, 2019
@sukhrajghuman sukhrajghuman deleted the feature/loading-overlay branch January 22, 2019 05:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants