Skip to content

Woo/aztec design changes#11594

Merged
cameronvoell merged 12 commits intodevelopfrom
woo/aztec-design-changes
Apr 24, 2020
Merged

Woo/aztec design changes#11594
cameronvoell merged 12 commits intodevelopfrom
woo/aztec-design-changes

Conversation

@anitaa1990
Copy link
Copy Markdown
Contributor

@anitaa1990 anitaa1990 commented Apr 6, 2020 •
edited
Loading

Fixes woocommerce/woocommerce-android#1750. This PR adds changes to the Aztec Editor to support customisation of the Aztec Toolbar.

Changes

  • Added multiple attributes to AztecToolbar:
    • toolbarBackgroundColor - to change the background color of the Aztec Toolbar.
    • toolbarBorderColor - to change the color of the divider above the Aztec toolbar.
    • toolbarIconNormalColor, toolbarIconHighlightColor, toolbarIconDisabledColor - to change the color of the Aztec toolbar button icons.
  • Since this PR adds support to change the color of the vector drawable icons used in the Aztec toolbar, I have removed all the drawables under WordPress-Editor module.

Note that this PR is in draft till this Aztec PR can be reviewed and merged and then we would need to update the hash to this branch before it can be merged.**

I understand the PR file size looks big but most of the changes are the drawable files that have been deleted from the WordPress-Editor module so I felt it shouldn't be a problem.

To test

  • Since this PR only makes changes to the styles, smoke testing the Aztec editor by selecting all the toolbar options should suffice.
  • Testing in API 21, 23, 28 and 29 would be great since the change affects all supported API versions. Without making any modifications to the app activity, the Aztec toolbar should look and function exactly as before.

Screenshot

API 21

. . .

API 23

. . .

API 29

. . .

PR submission checklist:

  • I have considered adding unit tests where possible.
  • 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.

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Apr 6, 2020 •
edited
Loading

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Apr 6, 2020 •
edited
Loading

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

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Apr 6, 2020 •
edited
Loading

You can test the changes on this Pull Request by downloading the APK here.

@anitaa1990 anitaa1990 marked this pull request as ready for review April 10, 2020 09:21
@anitaa1990
Copy link
Copy Markdown
Contributor Author

anitaa1990 commented Apr 10, 2020

PR is ready for review 👍

@0nko 0nko self-assigned this Apr 16, 2020
0nko
0nko reviewed Apr 19, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@0nko 0nko left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice code dump! I love it when a huge chunk of unused code gets removed :).

Things are looking solid, but I did find an issue where the List and Header buttons are highlighted (correctly) in dark mode, but not in light mode (on API 28):

Light mode Dark mode
image image

@anitaa1990
Copy link
Copy Markdown
Contributor Author

anitaa1990 commented Apr 20, 2020 •
edited
Loading

Thanks for the review @0nko!

Things are looking solid, but I did find an issue where the List and Header buttons are highlighted (correctly) in dark mode, but not in light mode (on API 28):

I noticed the same thing when pulling changes from develop so not sure if this has something to do with dark mode rather than changes with this PR.

@anitaa1990 anitaa1990 requested a review from 0nko April 20, 2020 10:01
0nko
0nko approved these changes Apr 20, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@0nko 0nko left a comment

Choose a reason for hiding this comment

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

Oh ok. Then this PR is a 👍 and I'll create an issue for the other problem.

@cameronvoell cameronvoell requested review from cameronvoell and removed request for cameronvoell April 20, 2020 17:31
@cameronvoell cameronvoell mentioned this pull request Apr 20, 2020
Merged
5 tasks
@cameronvoell cameronvoell self-requested a review April 21, 2020 01:01
cameronvoell
cameronvoell suggested changes Apr 21, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@cameronvoell cameronvoell left a comment

Choose a reason for hiding this comment

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

@ondrej @anitaa1990 I have this PR open for updating Aztec in gutenberg-mobile => wordpress-mobile/gutenberg-mobile#2170. Once that gets merged we should update the libs/gutenberg-mobile submodule reference in this PR to the merged gutenberg-mobile develop branch commit so that the Aztec version stays the same in the WordPress-Android and gutenberg-mobile.

@cameronvoell
Copy link
Copy Markdown
Contributor

cameronvoell commented Apr 24, 2020

@anitaa1990 gutenberg-mobile has it's Aztec version in sync with this PR now, so feel free to update the gutenberg-mobile submodule to 9abd4aca67e0a828034edf42871daee285d7b7bd, and everything should be good for merging 👍

@anitaa1990
Copy link
Copy Markdown
Contributor Author

anitaa1990 commented Apr 24, 2020

Thanks for the heads up @cameronvoell! I have updated the gutenberg-mobile hash to the PR and pulled in the latest changes from develop. PR ready for merge 👍

@hypest
Copy link
Copy Markdown
Contributor

hypest commented Apr 24, 2020

Looks like many of the CI jobs have failed due to the gutenberg-mobile build on JitPack failing. I've deleted the JitPack build to let it rebuild and restarted the CI jobs.

This was referenced Apr 24, 2020
Closed
Merged
@hypest
Copy link
Copy Markdown
Contributor

hypest commented Apr 24, 2020

Headsup: this PR is blocked by failing CI jobs because the gutenberg-mobile JitPack-built artifact/dependency fails to build. #11747 is trying to remedy and it has to target this PR anyway. So, let's merge #11747 first and then this one.

@cameronvoell
Copy link
Copy Markdown
Contributor

cameronvoell commented Apr 24, 2020 •
edited
Loading

So, let's merge #11747 first and then this one.

#11747 is Merged. So now we have shallow submodule enabled(which helped us pass CI), and Aztec versions in synch between WPAndroid and gutenberg-mobile. I did one last build of WPAndroid locally and everything looks good. Nice teamwork @hypest @anitaa1990 !

@cameronvoell cameronvoell merged commit 8c44ddc into develop Apr 24, 2020
@cameronvoell cameronvoell deleted the woo/aztec-design-changes branch January 28, 2021 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@cameronvoell cameronvoell cameronvoell approved these changes

@0nko 0nko 0nko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@0nko 0nko

Labels

Aztec

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Aztec editor: Design changes

4 participants

@anitaa1990 @cameronvoell @hypest @0nko