Skip to content

Making AlreadyReblogged add a checkmark to reblog icon.#1048

Merged
homu merged 5 commits intonew-xkit:masterfrom
Aquaj:already_reblogged_checkmark
May 18, 2016
Merged

Making AlreadyReblogged add a checkmark to reblog icon.#1048
homu merged 5 commits intonew-xkit:masterfrom
Aquaj:already_reblogged_checkmark

Conversation

@Aquaj
Copy link

@Aquaj Aquaj commented May 16, 2016

Following-up on #980 accessibility issue.

Made it possible to check the appropriate setting in One-Click Postage config to make AlreadyReblogged add a checkmark instead of just coloring the reblog button green.

m_button.addClass("reblogged");
if (XKit.extensions.one_click_postage.preferences.alreadyreblogged_use_checkmark.value) {
var checkmark = document.createElement("div");
checkmark.innerHTML = "✓";
Copy link
Member

@nightpool nightpool May 16, 2016

Choose a reason for hiding this comment

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

We should just be able to do with with CSS rather then super-expensive DOM manipulation, right?

@nightpool
Copy link
Member

This should basically be a 3 or 4 line CSS change, using pseudo-elements and content: "", right?

@Aquaj
Copy link
Author

Aquaj commented May 16, 2016

Actually that's what I tried first (planned on using a.reblogged::after) but for some reason I wasn't able to get ::after to work on .reblogged either in user stylesheet or in XKit's (and I tested those on both FF, Chrome and Vivaldi).

And since tumblr already uses the ::before for the reblog icon I elected to leave pseudo-elements aside and made it actual DOM.
If someone has an idea why my ::after sucked though, I'll change it up.

re: default behavior - I was thinking so too.
Opinion on whether we should we leave the option in or just add the checkmark and scrap choice for the sake of simplicity ?

@nightpool
Copy link
Member

hmm works for me....
image
image

.post_controls .post_control.reblog.reblogged:after {
    content: '✓';
    font-size: 18px;
    color: #56bc8a;
    opacity: 100;
    display: block;
    margin-top: 0px;
    margin-left: 0px;
    background: white;
    height: 10px;
    line-height: 10px;
}

Maybe you were getting bit in the butt by selector specificity? You have to pay real close attention to that stuff when coding for XKit.

Anyway, i'll merge if you get the CSS added and take out the option (since alreadyreblogged is a choice to begin with, I'm fine with scrapping customizability here for the sake of simplicity)

@nightpool
Copy link
Member

wrong button sorry :(

@Aquaj
Copy link
Author

Aquaj commented May 17, 2016

No problem we've all misclicked our way through stuff.
I'll do that when I get to my laptop again.

I'm tempted to say it was probably due to selector specificity but in User Stylesheet I actually used the same selector as tumblr/XKit for the ::before pseudo-element but eh. If I'm the only one whom for it doesn't work it'll be good enough 👍

… default behaviour instead of optional. See PR discussion.
@Aquaj
Copy link
Author

Aquaj commented May 18, 2016

Should be as discussed now 👌.

Kept the extraction of the m_button.addClass("reblogged"); to a method though, for DRY purposes and in prevision of the day we have to modify that again in the future.

@nightpool
Copy link
Member

LGTM! @homu r+

Be careful not to over-DRY your code though. The CSS class itself served as the proper abstraction layer here. Try to avoid premature refactoring of code that's already clear and readable.

@homu
Copy link

homu commented May 18, 2016

📌 Commit f68cb20 has been approved by nightpool

@homu
Copy link

homu commented May 18, 2016

⚡ Test exempted - status

@homu homu merged commit f68cb20 into new-xkit:master May 18, 2016
homu added a commit that referenced this pull request May 18, 2016
Making AlreadyReblogged add a checkmark to reblog icon.

Following-up on #980 accessibility issue.

Made it possible to check the appropriate setting in One-Click Postage config to make AlreadyReblogged add a checkmark instead of just coloring the reblog button green.
@Aquaj
Copy link
Author

Aquaj commented May 18, 2016

I get that, over-dry code ends up very... spaghetti-y but I do feel that line needed to be extracted to a method though.
It is to be applied on successful reblogging of the post or if the post is stored as already reblogged which are two separate paths that can easily get modified without regards to one another. (It wouldn't have made sense for example if the reblogging then called the #check_if_already_reblogged method again instead, as it would've used the path again.)

I'm sorry if I'm not really talking straight, it's 4am here and those code-design things are always bit of a subjective field.
Will keep that in mind though !

@Aquaj Aquaj deleted the already_reblogged_checkmark branch May 18, 2016 01:50
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.

4 participants