fix(comments): remove unused "btn-faded" class from buttons#1345
fix(comments): remove unused "btn-faded" class from buttons#1345MarskyMessier wants to merge 1 commit intolk-arpg:release/v3.0.0from
Conversation
|
Could you provide a screenshot of the differences before/after, please? iirc the outlines ended up making things a bit busy visually, but it has been some time since I last saw vanilla comments. |
itinerare
added
bug
SpeedyD
left a comment
SpeedyD
left a comment
There was a problem hiding this comment.
I admit having no idea what btn-faded is either and can't even find anything related to Bootstrap, but I disagree with adding outlines. It makes it feel busier, more crowded.
So I've added suggestions below, basically instead of outline, just make it text-secondary, text-info and text-success respectively, to make them stand out from one another without being busier. :)
| <div class="col-auto"> | ||
| @can('reply-to-comment', $comment) | ||
| <button data-toggle="modal" data-target="#reply-modal-{{ $comment->getKey() }}" class="btn btn-sm px-3 py-2 px-sm-2 py-sm-1 btn-faded text-uppercase"><i class="fas fa-comment"></i><span | ||
| <button data-toggle="modal" data-target="#reply-modal-{{ $comment->getKey() }}" class="btn btn-outline-secondary btn-sm px-3 py-2 px-sm-2 py-sm-1 text-uppercase"><i class="fas fa-comment"></i><span |
There was a problem hiding this comment.
I don't think setting btn-outline-secondary is that good an idea. May I suggest the following instead?
| <button data-toggle="modal" data-target="#reply-modal-{{ $comment->getKey() }}" class="btn btn-outline-secondary btn-sm px-3 py-2 px-sm-2 py-sm-1 text-uppercase"><i class="fas fa-comment"></i><span | |
| <button data-toggle="modal" data-target="#reply-modal-{{ $comment->getKey() }}" class="btn text-secondary btn-sm px-3 py-2 px-sm-2 py-sm-1 text-uppercase"><i class="fas fa-comment"></i><span |
| @endcan | ||
| @can('edit-comment', $comment) | ||
| <button data-toggle="modal" data-target="#comment-modal-{{ $comment->getKey() }}" class="btn btn-sm px-3 py-2 px-sm-2 py-sm-1 btn-faded text-uppercase"><i class="fas fa-edit"></i><span | ||
| <button data-toggle="modal" data-target="#comment-modal-{{ $comment->getKey() }}" class="btn btn-outline-secondary btn-sm px-3 py-2 px-sm-2 py-sm-1 text-uppercase"><i class="fas fa-edit"></i><span |
There was a problem hiding this comment.
Same here, frankly, but to have Edit stand out from Reply, how about this..
| <button data-toggle="modal" data-target="#comment-modal-{{ $comment->getKey() }}" class="btn btn-outline-secondary btn-sm px-3 py-2 px-sm-2 py-sm-1 text-uppercase"><i class="fas fa-edit"></i><span | |
| <button data-toggle="modal" data-target="#comment-modal-{{ $comment->getKey() }}" class="btn text-info btn-sm px-3 py-2 px-sm-2 py-sm-1 text-uppercase"><i class="fas fa-edit"></i><span |
Info is a lighter blue, it just feels softer :)
| @endcan | ||
| @if (((Auth::user()->id == $comment->commentable_id && $comment->commentable_type == 'App\Models\User\UserProfile') || Auth::user()->isStaff) && (isset($compact) && !$compact)) | ||
| <button data-toggle="modal" data-target="#feature-modal-{{ $comment->getKey() }}" class="btn btn-sm px-3 py-2 px-sm-2 py-sm-1 btn-faded text-success text-uppercase"><i class="fas fa-star"></i><span | ||
| <button data-toggle="modal" data-target="#feature-modal-{{ $comment->getKey() }}" class="btn btn-sm btn-outline-success px-3 py-2 px-sm-2 py-sm-1 text-uppercase"><i class="fas fa-star"></i><span |
There was a problem hiding this comment.
And this was already handled well, just remove btn-faded.
| <button data-toggle="modal" data-target="#feature-modal-{{ $comment->getKey() }}" class="btn btn-sm btn-outline-success px-3 py-2 px-sm-2 py-sm-1 text-uppercase"><i class="fas fa-star"></i><span | |
| <button data-toggle="modal" data-target="#feature-modal-{{ $comment->getKey() }}" class="btn btn-sm text-success px-3 py-2 px-sm-2 py-sm-1 text-uppercase"><i class="fas fa-star"></i><span |
|
I think no outline is fine, I personally added them as it was how the delete button was, and I think it might look a bit awkward that only delete & likes have the outline but not the rest of the buttons. I do like the decluttering however, I'm in for the text itself, just asking if maybe it would be more judicious to remove the outline from the delete button ? The screenshot below show both the outline changes (top) and Speedy's proposal (bottom). |
Personal reason is: The Delete button should stand out because out of the four buttons, it's the one that is the dangerous one, and the others technically just.. function and read as links instead. As for likes, it's.. detached from the rest. It does not perceive similar to the other buttons, so I feel like that one is fine by itself. Ironically, the '# Likes' text is also a button, but it doesn't stand out like the rest.. 🤷 |
|
This conundrum has given me thoughts on the overall UI of comments and trying to make them less cluttered/etc, so I'm going to fiddle around with ui/ux for comments when I have free time, beyond what you've suggested here. Thank you for the push to look into this! |
MarskyMessier
force-pushed
the
push-opvlqmluxqxq
branch
from
95fb4ed to
59f651a
Compare
|
I discovered while editing this that the v3 version of bootstrap is slightly older than dev, meaning that the
That's honestly fine ! After some thoughts I do think keeping the delete button as a button is better than anything, best to be cautious than anything. |
|
Let's see.. main is on v4.1.3, and so is release.. and develop is on v4.6.2.. ..From what I can see, both of those versions of Bootstrap should allow this, and.. I guess I see no problem with this? Should visually remain the same as the screenshot you did of my proposal? I'm currently not in the right mental state to properly test this, but from what I can tell it should work. I'll review in the morning. Wait..Why does my brain do this to me? Able to quickly research which versions we're on and whether or not Bootstrap allows this, but when it comes to just.. copy pasting a bit of code and testing, I'm suddenly too exhausted mentally.. what the heck.. |
SpeedyD
left a comment
SpeedyD
left a comment
There was a problem hiding this comment.
Alright, got to testing, there's only ONE minor thing you overlooked.
Call this an approve once that's finished. :)
| @can('reply-to-comment', $comment) | ||
| <button data-toggle="modal" data-target="#reply-modal-{{ $comment->getKey() }}" class="btn btn-sm px-3 py-2 px-sm-2 py-sm-1 btn-faded text-uppercase"><i class="fas fa-comment"></i><span | ||
| class="ml-2 d-none d-sm-inline-block">Reply</span></button> | ||
| <a data-toggle="modal" href="#reply-modal-{{ $comment->getKey() }}" class=" text-secondary px-3 py-2 px-sm-2 py-sm-1 text-uppercase btn-faded"><i class="fas fa-comment"></i><span |
MarskyMessier
force-pushed
the
push-opvlqmluxqxq
branch
from
59f651a to
d624118
Compare
That should now be fixed ! |
|
I think for the sake of keeping this PR scoped to it's original changes as closely as possible, I'm going to advocate for putting the "Reply" button back to grey. Unfortunately the text-info color isn't accessible: text-primary is slightly better but still doesn't meet the 4.5 ratio recommendation for keeping text accessible. To fix that properly any suggestions I would have, would be a massive amount of scope creep on this PR's original intentions and since it sounds like Uri is going to be looking into improving this UI in general anyway might be best to leave any unnecessary style adjustments for that endeavor. |
I agree with this, also for the sake of keeping this PR to its original scope. Can also confirm |
MarskyMessier
force-pushed
the
push-opvlqmluxqxq
branch
from
d624118 to
853aa7a
Compare
|
I reverted the changes from info back to secondary, so it's back to how the colors were originally |
itinerare
added
reviewed
| {{-- Likes Section --}} | ||
| <a href="#" data-toggle="modal" data-target="#show-likes-{{ $comment->id }}"> | ||
| <button href="#" data-toggle="tooltip" title="Click to View" class="btn btn-sm px-3 py-2 px-sm-2 py-sm-1 btn-faded"> | ||
| <a href="#" data-toggle="tooltip" title="Click to View" class="px-3 py-2 px-sm-2 py-sm-1"> |
There was a problem hiding this comment.
Noted that I've just noticed an issue with the like counter button where you cannot access it if the wrapped link is an a class instead of button.
Any suggestion to make that and the tooltip work is appreciated! Atm I haven't found a quick way to make it work with the tooltip.
There was a problem hiding this comment.
@MarskyMessier At this point I'd suggest closing this PR and remaking one to just remove the btn-faded class.
The scope of this PR has been to hell and back and even though the other changes are viable, they're also somewhat unnecessary.
If you do find them necessary, I'd suggest making new PRs for additional changes.
SpeedyD
left a comment
SpeedyD
left a comment
There was a problem hiding this comment.
Repeating my strong suggestion to recreate this PR, and thus removing my approval.
@SpeedyD I mentionned earlier in the replies why the scope went to changing it to another class. I'm happy to redo a PR, but this might cause issues down the line with |
|
Yes, I'll agree with that point: At Bootstrap 4.1.3 it's only outlines that got transparent backgrounds, and somewhere between that version and 4.6.2 they made it so that all btn classes have transparent backgrounds (unless a specific color is added)... But.. if the button transparency is the issue, why not just add the All you'd need to do then is.. effectively replace all the |
9 participants
This is a proposition to remove an unused class (
btn-faded) in a blade file pertaining to comments - I am unsure as to why or how that class wriggled its way in, but it's not used elsewhere nor referenced in the css files. (Best guess being ToyHouse code being referenced and forgotten at some point)Given it's not an "official" bootstrap color either, I replaced it by the secondary color and added back the outline to all buttons (reply, edit & feature). If it feels too crowded we could just make them links instead of buttons and tie text colors to them. Either way just a small fix from me yelling at clouds again.