ChessCom / browser-extension

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

Fix "Hide" option #70

Open emilgoldsmith opened 7 years ago

emilgoldsmith commented 7 years ago

Lots of unintended behaviour going on, I can list thoroughly if you want, but I'm currently working on fixing it, so the easiest way to show you is through a PR if I figure out what's going on exactly.

martynchamberlin commented 7 years ago

Ok! Also a screencast using something like http://quickcast.io/ is always helpful when describing a bug.

emilgoldsmith commented 7 years ago

Clarification:

hide-bugs

As can be seen, specific bugs I found are: Hide Friends is also removing New Game instead of friends, I located this to be, to be frank, horrendously wrong CSS selectors throughout the config file which I'll fix and submit a PR for.

Opening the hide option toggles some kind of reset, so even though in this case new game should be shown, pressing hide makes it show again until you toggle the button twice. I have found the bug to be a problem in the componentDidUpdate somehow, adding shouldComponentUpdate as in #71 has already fixed this issue (in my local fork). So I'll also add a bit more to that and submit a PR at some point, I'll also see if I can get a bit deeper of an understanding of the bug more than just adding shouldComponentUpdate as there may be a deeper underlying issue

emilgoldsmith commented 7 years ago

I just started looking at it a bit again, I'm assuming the CSS selectors used to work once though there's for example one that is .social-share-present with a comment saying this should be applicable to the /home site, which at least as a non-paying member is not even a class on the site (I used Chrome dev tools to do a query).

I'm assuming some of the problems could be with differences between members like for example adds that I have while paying members might not have them. But we definitely need to cover all edge cases where I guess cases are paying members, free members, and even non-logged in though it's very unlikely that someone would use the plugin without having a user.

I can definitely do better than has been done now, also because the specificity of things are completely off sometimes (having super specific ones and then super general selectors which all get "or'ed" so the general ones will delete things on other sites like for example #sidebar > section:nth-of-child(2) will hit something unintended on basically any site). Yeah I'm not writing way too structure right now, but I can definitely do better, but my overall message is, the easiest thing would definitely if from you guys (Chess.com)'s side you put ID's on all the elements you want to hit here, then CSS selectors would become a piece of cake.

emilgoldsmith commented 7 years ago

So yeah I've been doing some more CSS selector queries on chess.com and with for example the hide friends button I believe it's currently impossible to use pure CSS selectors to accurately select only Friends in a general way that works for all the pages.

So the easiest is definitely if you could add ID's from the Chess.com side of things. I could also make it work by adding an extra level to Config and further down the "tree" where you could make selector "cases" with regex for matching URLs. So that if it's /home we use this selector, otherwise use another one etc.

What are your thoughts @martynchamberlin ? Do you think adding ID's is plausible or should this project be completely separate?

If we added ID's and Chess.com consistently added these ID's on any new pages it would also be super future proof and a nice non-hardcoded solution which would be great.

martynchamberlin commented 7 years ago

Ok! Makes sense. Let me make a note to add proper IDs to these elements!

emilgoldsmith commented 7 years ago

That's great @martynchamberlin, that would make for some really nice code. Just imagine for example Chess.com makes a new page, or moves around some elements, and because of good ID's, the extension just keeps on working seamlessly :).

emilgoldsmith commented 7 years ago

Just added on hold until ID's are pushed to Chess.com production

martynchamberlin commented 7 years ago

@emilgoldsmith What aspect of the extension do we need to add IDs for? I'm wanting to put together a list of things that need IDs. Is it just the "Hide" section we're talking about?

emilgoldsmith commented 7 years ago

Hey @martynchamberlin. Awesome that you're taking initiative to get that done. Well I guess it would be awesome for everything really. Text, backgrounds and hide. It was just Hide that I on my initial play with the app noticed some quite gross bugs with.

But I'm assuming Chess.com would at some point love to have this extension be a production-ready marketable extension, and if you had consistent IDs on every element that is being targeted by the extension that would make everything super clean, bug-free and future proof.

Yeah so from my perspective I'd love to see all the selectors in Config.js be a single selector with either an ID or maybe a Class for things like backgrounds, but not all these "pseudo-cases" which are super bug-prone and end up either not targetting everything they should, or targetting something unintended, or breaking when Chess.com changes a bit of structure on the site or depending on whether you have ads etc (paying member or not).

So if I were you I'd go through the html of Chess.com and become very clear in the design decision of what exactly is it I want each extension button to target, and then make the CSS selectors for that super clean and simple.

martynchamberlin commented 7 years ago

Given that we're on the cusp of making a major change to the home page, I'm going to put this on hold until that's merged. Should be this week though!

emilgoldsmith commented 7 years ago

That's a great timeline for actually changing stuff on the main page, awesome :D.

emilgoldsmith commented 7 years ago

Also, just a friendly request in order to make sure the extension becomes high quality. When you assign the IDs take some time either with yourself or the design team of Chess.com to decide thoroughly exactly which elements you want each of the buttons on the extension to change, just so the customization part of the extension actually becomes a really nice, useful and accurate feature.

I guess it could be some of the options should also be changed / rearranged / renamed or anything else, not because I'm specifically suggesting that, it all depends on what design you guys decide on, I just think it'd be awesome if it became really consistent and accurate.

martynchamberlin commented 7 years ago

Good point! I've noticed that for example the "Post" button on Notes at /home does not change when you modify button design in the extension. We do need to standardize some stuff so it's easily targetable. I'm not sure that DOM Ids are the right solution by the way, for these reusable components. I'll probably move in favor of descriptive class names.

emilgoldsmith commented 7 years ago

Yeah definitely agree with the IDs versus Class Names when we get to things like colors and probably also partly hide options, as obviously you would have to have more than one element per page in some cases that was targeted by the same feature.

Descriptive class names sound awesome. In my vision I love anything that is accurate and unambiguous, so for example having descriptive class names with no CSS attached to them just to classify for the extension for example. That of course entails Chess.com putting in some "commitment" with the extension but I'm assuming that's also the plan for the future when it is more production ready.

emilgoldsmith commented 7 years ago

Also @martynchamberlin another small thing. When you change colors of some elements like for example in /home and you change the sidebar background color, I can see you're using CSS transitions on the Play a Friend / Custom Challenge etc. buttons which makes it look a bit weird and unprofessional.

I could imagine it wouldn't be easy to work around this though as I assume you use it for some other interactive animation when clicking them or something? But just wanted to let you know

martynchamberlin commented 7 years ago

Yeah I've been noticing that too. Definitely makes it look a little gross. :) And to be honest we used to do animation stuff but we aren't any more. So not a good reason for it to be there any more! I'll look into removing it.