webcompat / webcompat-reporter-extensions

Browser extensions to help report site compatibilty issues.
26 stars 21 forks source link

Fixes #131. Update classname to hide download link on webcompat.com. #132

Closed laghee closed 6 years ago

laghee commented 6 years ago

I went ahead and switched out the old classname link for the current one. Unit tests pass, and when I test in the command line of the console is-hidden is definitely coming up as part of the class list. But I can't get any of the practical testing methods to work in any of my firefox versions (or chrome), so I'm not confident something bigger isn't broken.

I've never gotten the dev dependency web-ext to work (as other noted in #101 / #105). So the last time I tested, I just added the unsigned addon in about:debugging, which worked fine. Now that's either messed up, too, or there's something fishy with the extensions.

There's definitely something amiss with web-ext + firefox (and/or in the way webpack bundles -- I keep getting "don't use eval(), it's evil" errors in ff console). I'd file a bug, but I can't narrow down the problem enough to know where to do so.

Installing web-ext globally or linking to source does start firefox with the addon, but there are some weird warnings/logging that pop up, and the addon keeps deactivating itself. Plus, w-e installs something like 900+ (npm global install) to 1300 packages (npm link from source), which, idk, seems ... like an awful lot? Maybe there's some kind of dependency conflict that's messing with function?

Anyway, here's this. If you have time and it works on your end, great, if not ... 🤷‍♀️ 😄

miketaylr commented 6 years ago

@laghee are you able to test locally?

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Temporary_Installation_in_Firefox

laghee commented 6 years ago

@miketaylr Nope. Sorry, too rambly up there. 🤓

What I meant -- it's not working at all with any method locally (web-ext or with about:debugging), so either something's hairy on my configuration, or I somehow broke the whole addon just by changing that one string (which seems melodramatic, but idk). After clicking it once, it grays out and turns unresponsive.

miketaylr commented 6 years ago

Ah. So, a million apologies for losing this PR.

.nav-item .nav-link {
    display: inline-block;
    padding: 5px;
    text-decoration: none;
}

.is-hidden {
    display: none;
}

Your addon is doing the right thing, we've just changed the CSS on webcompat.com. .nav-item .nav-link has higher specificity than .is-hidden, so it wins.

Maybe instead of adding a class, we use .hide() (which should do an inline "display: none" i think)

laghee commented 6 years ago

I really need to go do a JS refresher... 🙈

I couldn't get hide() to work (it kept throwing a console error that the function didn't exist), but I did manage to make the dratted thing disappear by using .style.display = "none". Not as elegant, but works.

The only weird side effect is that it seems to cause/reveal/expose that same gap from WC.com Issue #2576 in both Firefox & Chrome.

screenshot 2018-09-26 18 47 25

Going to push what I have, @miketaylr. No rush... lol ;p

Edit: We'll have to remember to update all the addon stores once this is changed.

miketaylr commented 6 years ago

I couldn't get hide() to work

Oh right. That's because it's a jQuery method... it won't work if you grab the element via querySelector (sorry about that). Changing the style directly is effectively the same thing. I don't have strong opinions one way or the other.

miketaylr commented 6 years ago

The only weird side effect is that it seems to cause/reveal/expose that same gap from WC.com Issue #2576 in both Firefox & Chrome.

Interesting. I wonder if @magsout can help us figure this out next week in Paris.

magsout commented 6 years ago

@miketaylr I'll take look, reping me next week ;)

magsout commented 6 years ago

@laghee PR landing https://github.com/webcompat/webcompat.com/pull/2609