ChessCom / browser-extension

Customize your Chess.com experience
Other
48 stars 15 forks source link

Remove the page reload when pressing reset #80

Closed emilgoldsmith closed 7 years ago

emilgoldsmith commented 7 years ago

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.

emilgoldsmith commented 7 years ago

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 commented 7 years ago

Gotcha. Carry on!

emilgoldsmith commented 7 years ago

Cool.

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

martynchamberlin commented 7 years ago

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

emilgoldsmith commented 7 years ago

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

emilgoldsmith commented 7 years ago

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:

emilgoldsmith commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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?

emilgoldsmith commented 7 years ago

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

emilgoldsmith commented 7 years ago

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.