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

Chore: Replace loading doc loading animation with a GIF #923

Merged
merged 7 commits into from
Feb 21, 2019
Merged

Chore: Replace loading doc loading animation with a GIF #923

merged 7 commits into from
Feb 21, 2019

Conversation

jeremypress
Copy link
Contributor

@jeremypress jeremypress commented Feb 12, 2019

Performance diff on IE11
Before:
capture2

After:
capture1

Todo:

  • perf / memory comparison with change
  • fix GIF on non-white backgrounds

screen shot 2019-02-20 at 1 59 30 pm

  • optimize GIF (currently ~800bytes)
  • tests

@boxcla
Copy link

boxcla commented Feb 12, 2019

Verified that @jeremypress has signed the CLA. Thanks for the pull request!

src/lib/_loading.scss Show resolved Hide resolved
.bp .bp-doc.bp-doc-document {
.pdfViewer .page .loadingIcon {
background: url('icons/loading.gif') center no-repeat;
background-size: $spinner-size $spinner-size;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be folded into background above.

src/lib/_loading.scss Show resolved Hide resolved
width: 30px;
.bp-document-preload-wrapper {
.bp-preload-spinner {
background: url('icons/loading.gif') center no-repeat;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use SCSS mixins to avoid repeating styles. Something like:

@mixin spinner() {
    background: url('icons/loading.gif') center $spinner-size $spinner-size no-repeat;
}

.bp .bp-doc .pdfViewer .page .loadingIcon {
    @include spinner;
}

.bp-preload-spinner {
    @include spinner;
    // other rules
}

jstoffan
jstoffan previously approved these changes Feb 12, 2019
@ConradJChan
Copy link
Contributor

ConradJChan commented Feb 12, 2019

Can we remove the styles related to the animation of the div? nvm, was looking at a subset of the changes

ConradJChan
ConradJChan previously approved these changes Feb 12, 2019
@jeremypress jeremypress merged commit 35fb9f7 into box:master Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants