Skip to content

"Activate" vs "Active" Wording on Theme Customization #17107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
mokagio merged 4 commits into from
Sep 9, 2021
Merged

"Activate" vs "Active" Wording on Theme Customization #17107

mokagio merged 4 commits into from
Sep 9, 2021

Conversation

@illusaen
Copy link
Contributor

@illusaen illusaen commented Sep 1, 2021

Using theme.isCurrentTheme to check whether wording on menu bar for "Try and Customize" theme screen should be "Activate" or "Active" instead of always using "Active".

Fixes #17060

Screenshot

Simulator.Screen.Recording.-.iPhone.12.mini.-.2021-09-01.at.12.36.20.mp4

Testing

To test:

  1. Go to Themes

  2. Click menu bar for an inactive theme

  3. Choose "Try and Customize" on bottom sheet

  4. Notice that top right menu bar button should say "Activate"

  5. Go to Themes

  6. Click menu bar for active theme

  7. Choose "Try and Customize" on bottom sheet

  8. Notice that top right menu bar button should say "Active"

Regression Notes

  1. Potential unintended areas of impact
    Using "Activate" on active themes.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Tested active theme's customization screen as well.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@illusaen illusaen added this to the 18.2 milestone Sep 1, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Sep 1, 2021 •
edited
Loading

You can trigger an installable build for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Sep 1, 2021 •
edited
Loading

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@illusaen illusaen requested a review from enejb September 1, 2021 17:43
@mokagio
Copy link
Contributor

mokagio commented Sep 6, 2021

@illusaen @enejb I'm code freezing 18.2 today, but this PR hasn't been reviewed yet.

I normally bump PRs that are not ready to the next release, but this being a bug fix I think it's worth keeping for 18.2. I'd like @startuptester opinion on this, too.

I am subscribed to this PR so I'll know if and when it merges and I'll ship a new beta including it when that happens. Or, if we think it can wait till the next release, I'll take care of updating the milestone. 👍

@illusaen
Copy link
Contributor Author

illusaen commented Sep 7, 2021

@illusaen @enejb I'm code freezing 18.2 today, but this PR hasn't been reviewed yet.

I normally bump PRs that are not ready to the next release, but this being a bug fix I think it's worth keeping for 18.2. I'd like @startuptester opinion on this, too.

I am subscribed to this PR so I'll know if and when it merges and I'll ship a new beta including it when that happens. Or, if we think it can wait till the next release, I'll take care of updating the milestone. 👍

Thanks much!

enejb
enejb approved these changes Sep 7, 2021
View reviewed changes
Copy link
Contributor

@enejb enejb left a comment

Choose a reason for hiding this comment

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

I tested this and it worked as described.
And the wording makes sense now. Thanks for the fix!

@enejb
Copy link
Contributor

enejb commented Sep 7, 2021 •
edited
Loading

Thanks for the fixes @illusaen !

During my testing, there are a couple of other things that could be improved but I think those changes can be tacked in different PRs.

  1. When you are customizing the site. The header says "Activate" but pressing it doesn't do anything.

Screen Shot 2021-09-07 at 12 14 30 PM

I am not sure but maybe we can just remove that button completely.
  1. After activating a new theme. You are presented with this pop up.

Screen Shot 2021-09-07 at 12 16 15 PM

I think we should present the user with something else instead.
Clicking Manage site doesn't really work as expected. The pop up closes the same thing happens when I click OK.

  1. When the theme is being activated there is no progress indicator.
  2. The "Active" Label is a bit hard to read.

Screen Shot 2021-09-07 at 12 21 11 PM

cc: @iamthomasbishop Would love to know how we should tackle some of the issues that I found here.
They could definitely be their own tickets here.

@mokagio
Copy link
Contributor

mokagio commented Sep 9, 2021

I'm planning to ship a new beta today to start testing #17130.

I'll be "taking over" this PR to get it in the beta. Whenever we can batch fixes into betas we try to do it, as deploying a beta involves quite a bit of extra communications downstream.

@mokagio mokagio changed the base branch from develop to release/18.2 September 9, 2021 01:42
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Sep 9, 2021 •
edited
Loading

Warnings
⚠️

This PR contains changes to RELEASE_NOTES.txt.
Note that these changes won't affect the final version of the release notes as this version is in code freeze.
Please, get in touch with a release manager if you want to update the final release notes.

Generated by 🚫 dangerJS

@mokagio
Merge remote-tracking branch 'origin/release/18.2' into theme-active-…
d9112e0 
…vs-activate

GitHub couldn't automatically resolve a conflict on the
`RELEASE-NOTES.txt` file, but my local Git auto-merged it without
trouble...

```
Auto-merging RELEASE-NOTES.txt
Merge made by the 'recursive' strategy.
```
@mokagio
Remove redundant entry from RELEASE-NOTES.txt
78058a0 
In d9112e0, I boasted about how my
local Git was able to automatically resolve merge conflicts on
`RELEASE-NOTES.txt`. Too bad it did it in a way that was not appropriate 🤦‍♂️

I should have checked that before pushing the commit... This addresses
that little issue.
mokagio
mokagio reviewed Sep 9, 2021
View reviewed changes
RELEASE-NOTES.txt
-----
* [internal] Fixed an issue where source and platform tags were not added to a Zendesk ticket if the account has no blogs. [#17084]
* [*] Set the post formats to have 'Standard' first and then alphabetized the remaining items. [#17074]
* [*] Fixed wording of theme customization screen's menu bar by using "Activate" on inactive themes. [#17060]
Copy link
Contributor

@mokagio mokagio Sep 9, 2021

Choose a reason for hiding this comment

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

@illusaen note that this entry won't make it into the release notes in the App Store, because those have already been submitted for translation by #17131.

Given the content, I think that's okay, but let me know if you think otherwise.

@mokagio mokagio merged commit f9f8f1c into release/18.2 Sep 9, 2021
@mokagio mokagio deleted the theme-active-vs-activate branch September 9, 2021 04:49
@mokagio mokagio mentioned this pull request Sep 9, 2021
Merged
@mokagio
Copy link
Contributor

mokagio commented Sep 9, 2021

@illusaen this has been bundled as part of 18.2 beta 1 (18.2.0.1).

Thanks for your work 🙌

@guarani guarani mentioned this pull request Oct 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mokagio mokagio mokagio left review comments

@enejb enejb enejb approved these changes

Assignees

No one assigned

Labels

[Type] Bug

Projects

None yet

Milestone

18.2

Development

Successfully merging this pull request may close these issues.

Themes > Try and Customize screen always says "Active" when it should say "Activate"

4 participants

@illusaen @mokagio @enejb