Closed ryelle closed 1 month ago
Considering this as a change that should apply across the site, not just the Themes section, I wonder: is it just the permalink for Themes that has this more verbose version? Mainly noting that in some cases, the star (in the mockups that's still the favorite icon, I'm aware there's a separate conversation on whether it's star or heart) stands alone in list views:
Adding the text on the permalink page alone, also given it has a border around it, can work. How does this work in mobile contexts? Can it be "Favorite" and "Favorited"?
On themes, we only show the button on single theme pages (that mockup you've shared was for the old full-redesign, I referenced it but didn't follow it for the refresh). This PR is only for the themes favorite button.
How does this work in mobile contexts?
The mobile screenshot is in the PR description, it's under the 2 desktop screenshots. It works exactly the same as desktop, it just wraps under the title.
If you want to have a conversation about unifying the favorite button, I've created a discussion here.
Do you have any requested changes for this PR, @jasmussen?
Oh my apologies, long day yesterday.
I think the discussion would be good to solve before moving forward, there are a few pieces here, across the icon, the label, showing numbers, etc.
@jasmussen Just to be clear, you're saying that solving the "unified favorite button" blocks this PR? do you think it would block launching the theme refresh as well?
Just the PR could benefit from weighting. I prefer not to block, but if we know we eventually want to unify the appearance of this particular button across the three directories, it'll be misleading to diverge in the interim. So I'd suggest waiting, if you can. I know @fcoveram is busy with another thing today, but otherwise his input on the GH discussion I would appreciate.
I can wait for more input, but I do just want to say— the work here is already done, and if this is an improvement on what's on production, I'd like to merge it. I had planned to come back to the "unify __ blocks" tasks after wrapping this iteration of the theme directory, as I mentioned at the end of the discussion topic there are other technical concerns that could make a unified favorite button take a bit longer to build, plus then deploying it out to all 3 sites.
I don't see it as an improvement. It doesn't wrap as well on mobile, and the increased prominence isn't clearly a benefit. And if we do mean to show like-counts across dirctories, and I expect we do, then it's not a benefit to merge this.
I don't mean it to sound dismissive of the effort, which I appreciate.
I do think this PR is an improvement.
It doesn't wrap as well on mobile, and the increased prominence isn't clearly a benefit.
The current favorited action of a pattern looks like this.
We can remove the label and keep the border style to make it look similar to Patterns. The report #79 points out a floating icon that feels detached from the page and this PR addresses that.
I have pending replying to the "unifying the fav action" discussion with a design that involves fav/liking, saving, and rating actions across the site, but that would take more time and should not block this work.
And if we do mean to show like-counts across dirctories, and I expect we do, then it's not a benefit to merge this.
As I mentioned in the discussion, we don't currently have like-counts on the other directories. It would likely be more "divergent work" to try to add that here now, when we don't have a unified design yet.
We can remove the label and keep the border style to make it look similar to Patterns. The report #79 points out a floating icon that feels detached from the page and this PR addresses that.
That works for me, should I update the PR?
We can remove the label and keep the border style to make it look similar to Patterns.
To iterate on this idea slightly, what if we did this and then repositioned the button slightly? Something like:
Proposed | Patterns Directory |
---|---|
This way, it looks pretty close to Patterns, and then we can reassess in the future with a unified design for favoriting.
Let's go with the suggestions—the bordered box would fit the number of likes if we choose to show that. Let's also follow up with a decision on whether to use hearts or stars. I'm in favor of the stars, but if you all want to go with the hearts, that's fine too. If we do that, hearts for all favoriting actions, and we can use the new SVGs here.
Okay, here's what we've got in this PR now:
Favorited | Unfavorited |
---|---|
This looks great to me 🚀
Fixes #79 — This adds a visible label next to the :heart: icon for favoriting themes. The label is "Add to favorites" or "Favorited" depending on the favorite status. The screen reader label is still the action ("Add to favorites" and "Remove from favorites"), and I've also added an announcement on success to let the screen reader user know that the add/remove succeeded.
Screenshots
On small screens, the button wraps now.
How to test the changes in this Pull Request:
Try the same with a screen reader to hear the announcement.