Conversation
|
Nice! You figured out the toolbar styling! That's amazing. Now I'm wondering how I missed it. Let me know when this one is ready for an official review. |
|
Thanks @AmandaRiu! This PR is in draft till this Aztec PR changes have been reviewed. |
Oh! Ok now it all makes better sense. You've added those changes to the aztec library. Very cool. Nice work! |
|
You can test the changes on this Pull Request by downloading the APK here. |
|
@AmandaRiu PR ready for review 👍 |
AmandaRiu
left a comment
There was a problem hiding this comment.
@anitaa Thank you for taking care of styling this toolbar. There are some code comments below to point out places where the deprecated values/files have been used.
After a close review of the changes and style options available I think we should try to style the AztecEditor toolbar exactly the same as the bottom bar in the main screens for consistency. I've made a few changes to how we're organizing the styles due to the way the styling is implemented in the AztecEditor side of things that requires overriding the library’s AztecToolbarStyle directly and doesn’t allow for extending it for customized styles or defining a theme-level style. The changes to support theming properly would be small so maybe we can get that added later. Either way it’s not a big deal.
I’ve created a new temporary branch with the recommend changes amanda/1750-aztec-design-changes (click on the link to view the diff). These include the following:
- Add all style definitions to the
AztecToolbarStylestyle and move it to the newstyles_base.xmlfile. - Remove the custom toolbar style and remove the style definition from the layout file.
- Update the colors of available options to match the bottom navigation bar.
Here’s how these changes look. WDYT?
| Before | After |
|---|---|
 |
 |
 |
 |
 |
 |
 |
 |
| android:layout_width="match_parent" | ||
| android:layout_height="@dimen/min_tap_target" | ||
| app:toolbarBackgroundColor="@color/color_surface" | ||
| app:toolbarBorderColor="@color/wc_border_color" |
There was a problem hiding this comment.
This is a color from the now deprecated color file wc_colors.xml. The new colors are in the following files:
values/wc_colors_base.xml- these are the palette colors and should contain only hexidecimal color valuesvalues/colors_base.xml+values-night/colors_base.xml- the light/dark color assignments and should use the colors defined inwc_colors_base.xml.
| <style name="AztecToolbarStyle"> | ||
| <item name="advanced">false</item> | ||
| <item name="toolbarIconNormalColor">@color/color_on_surface_medium</item> | ||
| <item name="toolbarIconHighlightColor">@color/color_primary</item> | ||
| <item name="toolbarIconDisabledColor">@color/color_on_surface_disabled</item> | ||
| </style> |
There was a problem hiding this comment.
This style has been added to the deprecated style file styles.xml. All new styles should be added to values/styles_base.xml.
There was a problem hiding this comment.
Oops! My bad! I added everything to deprecated files 🤦♀️! Fixed in 7c91e3b
|
Thanks for the review @AmandaRiu! I'm good to make those changes to Aztec given that it's consistent with how our bottom navigation bar is displayed. I think it warrants a design review from @Garance91540 since this modifies the designs a little bit :D |
|
Fixed the requested changes @AmandaRiu! PR ready for another round 👍 |
Fixes #1750 by adding design changes to the Aztec Toolbar editor.
Note that this PR is in draft till this Aztec PR is merged and then we would need to update the hash to this branch before it can be merged.I decided to implement this change in the dark mode branch since it updates styles and colors to the Aztec editor.
Screenshots
Light Mode:
Dark mode
To test
Update release notes:
RELEASE-NOTES.txtif necessary.