Client QA Updates - Githubissues

INN / umbrella-citylimits

CityLimits.org Site

https://citylimits.org

GNU General Public License v2.0

0 stars 2 forks source link

Client QA Updates #133

Closed MirandaEcho closed 4 years ago

MirandaEcho commented 4 years ago

[x] Add newsletter signup to "News" (category landing page)
[x] Mapping the Future should be checked by default in the Newsletter signup
[x] Remove lede image from author landing page template
[x] Change "And" to "and" on article page main bylines
[x] Reduce size of top headline on mobile homepage so its more in line with headlines for subsequent stories
[x] Update byline size on series landing pages so that it matches the sizes on category landing pages
[x] Change email share to use this format:

Email Subject is: CityLimits.org: [HEADLINE]

Email Body is: From City Limits: [HEADLINE] [DEK] [Link]

joshdarby commented 4 years ago

@benlk Do you know if $checkedFlag here is supposed to reset to false for every item in $newsletter? Right now it sets the first item to true and causes every item afterwards to be set to true as well.

https://github.com/INN/umbrella-citylimits/blob/0524a414608499780eede6d186e09f144205c031/wp-content/themes/citylimits/partials/newsletter-signup-maincolumn.php#L25-L48

benlk commented 4 years ago

I assume it's supposed to reset to false, but it isn't for reasons that look like there's logic that wasn't completed. This is a partial that I did some code-styles cleanup on before we merged https://github.com/INN/umbrella-citylimits/pull/33, but it looks like I missed the checkbox logic.

Would it make more sense to modify the fields to have a "checked by default?" option, which determines whether each newsletter should be checked? This would complement the "active?" option, and could use https://developer.wordpress.org/reference/functions/checked/ to output the checked params.

joshdarby commented 4 years ago

Would it make more sense to modify the fields to have a "checked by default?" option, which determines whether each newsletter should be checked?

@benlk Yep, that's what I was thinking. Just wanted to make sure that this was a bug instead of intended behavior before I modified it.

MirandaEcho commented 4 years ago

@joshdarby - the top story headline on mobile is still too large. It should pretty much match the headlines on the rest of the mobile page.

joshdarby commented 4 years ago

[x] the top story headline on mobile is still too large. It should pretty much match the headlines on the rest of the mobile page.
[x] Fix new series landing template widget issue, detailed here: https://github.com/INN/largo/issues/1839
[x] Allow selection of empty secondary nav on landing pages
[x] Research why Jarrett is not seeing "Series" as an option on the menu tool
[x] Remove hardcoded newsletter signup from new series template so they can use the widget instead
[x] Allow selection of different menu for open toggled mobile nav
[x] Hide mobile newsletter popup on all pages, not just homepage

joshdarby commented 4 years ago

@MirandaEcho The reason Jarrett is not seeing the "Series" option when creating menus is probably because it's not checked under "Screen Options" for his account. I logged into his account on my local environment, checked the box, and the option appears now.

Screen Shot 2019-12-17 at 10 57 47 AM

Should I make that change on his staging account or should we just send him that screenshot and attached instructions on how to do it himself?

joshdarby commented 4 years ago

@MirandaEcho That's been updated on his account on the staging environment.