feat: make My Account button only show icon on mobile#406
feat: make My Account button only show icon on mobile#406laurelfulford merged 5 commits intotrunkfrom
Conversation
b6f279e to
e84e192
Compare
|
@laurelfulford I updated the patterns based on the new classname approach. |
rbcorrales
left a comment
There was a problem hiding this comment.
Tested all three mobile header styles with RAS enabled and compared against trunk. Left inline comments with questions on a few things I noticed. Not all of them might necessarily be issues.
|
@rbcorrales I’ve updated all the mobile patterns. The if/else checks were intentional, but on reflection they don’t really make sense. I’d rather offer a few more patterns with small variations than rely on conditional logic like that. I also removed the “Donate” label from the button. I think that could have been confusing for publishers, since it looked like the button would work out of the box, which isn’t the case as they still need to add a link. With no default label, they can now customise it to whatever they want (Donate, Subscribe, etc...) and add a link as needed. Oh, and I made sure the My Account button block can’t be removed. |
|
Note: we’ll need to update the desktop patterns as well, but that can be done in a separate PR. |
rbcorrales
left a comment
There was a problem hiding this comment.
Thanks for addressing all the feedback @thomasguillot! Looks good. The new approach of offering more pattern variations instead of conditional logic is cleaner.
|
Sweet! Thanks for taking care of all those fixes, @thomasguillot, and for the review, @rbcorrales! I'm going to go ahead and merge this one! |
All Submissions:
Changes proposed in this Pull Request:
This either needs to be tested with Automattic/newspack-plugin#4443, or after it's merged.
See NPPD-1183.
How to test the changes in this Pull Request:
Other information: