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

Remove the page reload when pressing reset #80

Merged
merged 13 commits into from
Mar 24, 2017

Conversation

emilgoldsmith
Copy link
Contributor

This PR will end up removing the need for a page reload when pressing reset by implementing the approach outlined in #63. (resolves #63)

Currently I have just added the initial commit to check in with @martynchamberlin and anyone else that my starting approach looks okay.

Copy link
Member

@martynchamberlin martynchamberlin left a comment

Choose a reason for hiding this comment

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

Hmm for some reason I can't get it to work! Clicking "reset all" now doesn't cause the page to reload (yay!) but I also cannot actually make visual changes! With the changes I've made in this screenshot, the "Create Challenge" widget should be all red, but it's not.

@emilgoldsmith
Copy link
Contributor Author

Oh yeah, sorry, I think you misunderstood me!

I didn't get that far yet. I simply successfully added a class to all the initially injected styles, and successfully added that class to the body element before initial render by using mutation observers. I haven't added the actual showing / not showing functionality yet. I just wanted to check in with you before I kept working that this was the kind of approach you were thinking of?

So there's nothing to physically test just as of yet, just code to look at.

@martynchamberlin
Copy link
Member

Gotcha. Carry on!

@emilgoldsmith
Copy link
Contributor Author

emilgoldsmith commented Mar 18, 2017

Cool.

So this was the kind of approach you were thinking of using in #63 yeah?

@martynchamberlin
Copy link
Member

Yeah I hadn't thought of your solution but it's brilliant and solves the problem in an elegant way.

@emilgoldsmith
Copy link
Contributor Author

Awesome. It was simply my interpretation of what you suggested in #63 haha!

@emilgoldsmith
Copy link
Contributor Author

Still very much a work in progress, but now the reset all button correctly removes all applied inline styles (it's currently assuming that Chess.com don't apply any themselves as it simply clears all inline styles, I'm assuming that's a correct assumption?)

Todo:

  • After load complete actually remove the temp stylesheet and apply all the inline styles, I already started writing this code
  • Integrate the rest of the reset buttons for the colorpicker into my new design
  • Hunt down any other page reloads every being requested like for example the font one.

@emilgoldsmith
Copy link
Contributor Author

Okay, I seem to have the basics working now! Any thoughts @martynchamberlin ?

I'm still lacking point 2 and three from above. But normal style changing and the reset all button should be working as intended now!

@martynchamberlin
Copy link
Member

Looking great. I'm noticing that we're reloading the whole page on an individual reset button click. Maybe we can remove that too in this PR?

@emilgoldsmith
Copy link
Contributor Author

Yep that's still on the to do as mentioned above about point two and three:

  • Integrate the rest of the reset buttons for the colorpicker into my new design
  • Hunt down any other page reloads every being requested like for example the font one.

@emilgoldsmith
Copy link
Contributor Author

The two above points have now been implemented!

Basically there now, I might do a few more checks and have a few edge cases I'd like to test at some point before we merge it though.

But as always, feel free to play around with it / check out the code and give any comments you might have.

@emilgoldsmith
Copy link
Contributor Author

I think I'm happy with this PR now, I opened some issues #92 and #93 which are things I thought of while doing this, but also think they aren't necessarily directly related to this PR so made them issues instead. I believe the PR now has achieved what it set out to achieve ;).

@martynchamberlin if you could look through it a final time and review if all is good?

Copy link
Member

@martynchamberlin martynchamberlin left a comment

Choose a reason for hiding this comment

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

Beautiful! I checked it in FireFox and Chrome and it's amazing. Huge improvement. Let's ship this.

@emilgoldsmith
Copy link
Contributor Author

Awesome I'll get around to fixing conflicts with master and get it merged in.

@emilgoldsmith
Copy link
Contributor Author

That was quite a hairy merge but I'm quite certain I got it all, tested it and looked through the new diff here on the PR after, so I'll merge it in.

@emilgoldsmith emilgoldsmith merged commit 14ab972 into master Mar 24, 2017
@emilgoldsmith emilgoldsmith deleted the remove-reset-styles-reload branch March 24, 2017 15:56
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.

Avoid page refresh whenever a style reset occurs
2 participants