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

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

Merged
merged 2 commits into from
Sep 26, 2018

Conversation

laghee
Copy link
Contributor

@laghee laghee commented Jul 14, 2018

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 ... 🤷‍♀️ 😄

@laghee laghee requested a review from miketaylr July 14, 2018 19:12
@miketaylr
Copy link
Member

@laghee
Copy link
Contributor Author

laghee commented Jul 17, 2018

@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
Copy link
Member

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
Copy link
Contributor Author

laghee commented Sep 26, 2018

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
Copy link
Member

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
Copy link
Member

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.

@miketaylr miketaylr merged commit bb47dcc into webcompat:master Sep 26, 2018
@magsout
Copy link
Member

magsout commented Sep 26, 2018

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

@magsout
Copy link
Member

magsout commented Oct 1, 2018

@laghee PR landing webcompat/webcompat.com#2609

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.

3 participants