Skip to content
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

Pass active state to HeadingLevelIcon in order to native start working #17747

Merged
merged 3 commits into from
Oct 3, 2019
Merged

Pass active state to HeadingLevelIcon in order to native start working #17747

merged 3 commits into from
Oct 3, 2019

Conversation

marecar3
Copy link
Contributor

@marecar3 marecar3 commented Oct 3, 2019

Description

Fixes: Heading block Hx (level) buttons won't change icon color when active RN mobile issue: wordpress-mobile/gutenberg-mobile#1398

How has this been tested?

GB mobile:

  1. Open a new post
  2. Add new heading block
  3. Active heading buttons in the toolbar should be presented with white text and inactive buttons should be presented with gray text.

Gutenberg:

Make sure that there isn't active tag in rendered html

Screenshots

heading

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@marecar3 marecar3 requested review from gziolo and etoledom October 3, 2019 12:13
@gziolo gziolo mentioned this pull request Oct 3, 2019
Merged
7 tasks
@gziolo
Copy link
Member

gziolo commented Oct 3, 2019

It looks good as a temporary workaround, however we need to figure out how to avoid blacklisting active in SVG implementation for the web.

youknowriad
youknowriad reviewed Oct 3, 2019
View reviewed changes
packages/block-library/src/heading/heading-level-icon.js Outdated
@@ -17,7 +18,7 @@ export default function HeadingLevelIcon( { level } ) {
}

return (
<SVG width="20" height="20" viewBox="0 0 20 20" xmlns="http://www.w3.org/2000/svg">
<SVG width="20" height="20" viewBox="0 0 20 20" xmlns="http://www.w3.org/2000/svg" active={ active } >
Copy link
Contributor

@youknowriad youknowriad Oct 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I understand why the "SVG" component should have an "active" prop. it would make sense for a "button" to be active but not the SVG component in my opinion (for both web and mobile)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @youknowriad @gziolo as I remember gutenberg is using some css magic which allows you to define heading text color in higher-level CSS which will be passed to SVG but native doesn't support that magic. Probably we can find some better solution? But this issue is blocking our release process so we can do it after the release? Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, with CSS you can use the class names applied to parents to change how it looks. With RN it isn't possible, that's why you need to pass it down as prop.

@gziolo gziolo added Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Package] Block library /packages/block-library labels Oct 3, 2019
@daniloercoli
Copy link
Contributor

Works as expected on mobile. Didn't merge this PR, since we want to be sure it's ok shipping this workaround.

gziolo
gziolo approved these changes Oct 3, 2019
View reviewed changes
Copy link
Member

@gziolo gziolo 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.

Let's ship and fix it in the release branch. If it is going to be merged in the master, let's ensure it's resolved with a more appropriate solution.

I started #17748 where we should continue the work. I will need some help with debugging though.

youknowriad
youknowriad previously requested changes Oct 3, 2019
View reviewed changes
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

At least, it should be marked as unstable https://developer.wordpress.org/block-editor/contributors/develop/coding-guidelines/#experimental-and-unstable-apis

@marecar3
Copy link
Contributor Author

marecar3 commented Oct 3, 2019

Hey @gziolo @youknowriad I have marked active as __unstableActive
This PR is ready for another round of review. Thanks!

@marecar3 marecar3 requested a review from youknowriad October 3, 2019 13:50
@youknowriad youknowriad dismissed their stale review October 3, 2019 13:51

Addressed

@marecar3 marecar3 merged commit e7bfe8e into rnmobile/release-v1.14.0 Oct 3, 2019
@marecar3 marecar3 deleted the rnmobile/fix-1398_Heading-block-Hx-(level)-buttons-wont-change-icon-color-when-active branch October 3, 2019 13:57
@marecar3 marecar3 mentioned this pull request Oct 3, 2019
Closed
etoledom added a commit that referenced this pull request Oct 8, 2019
@marecar3 @etoledom
[RNMobile] Native mobile release v1.14.0 (#17676)
2be122f 
* Rnmobile/fix link editing on start (#17631)

* Don't try to clear links if text is clean.

* Commented LinkUI removal test when no URL.

* Don't try to remove link if we are at start of link and no actual selection is

* Mobile master to master (#17652)

* [RNMobile] Native mobile release v1.11.0 (#17181)

* [RNMobile] Fix crash when adding separator

* Build: remove global install of latest npm since we want to use the paired node/npm version (#17134)

* Build: remove global install of latest npm since we want to use the paired node/npm version
* Also update travis to remove --latest-npm flag

* [RNMobile] Try dark mode (iOS) (#17067)

* Adding dark mode component implemented on list and list block

* Adding DarkMode handling to RichText, ToolBar and SafeArea

* Mobile: Using DarkMode as HOC

* iOS DarkMode: Modified colors on block list and block container

* iOS DarkMode: Improved Header Toolbar colors

* iOS DarkMode: Removing background from buttons

* iOS DarkMode warning and unsupported

* iOS DarkMode: MediaPlaceholder

* iOS DarkMode: BottomSheets

* iOS DarkMode: Inserter

* iOS DarkMode: DefaultBlockAppender

* iOS DarkMode: PostTite

* Update hardcoded colors with variables

* iOS DarkMode: Fix bottom-sheet cell value color

* iOS DarkMode: More - PageBreak - Add Block Here

* iOS DarkMode: Better text color

* iOS Darkmode: Code block

* iOS DarkMode: HTML View

* iOS DarkMode: Improve colors on SafeArea

* Fix toolbar not avoiding keyboard regression

* Fix native unit tests

* Fix gutenberg-mobile unit tests

* Adding RNDarkMode mocks

* RNMobile: Fix crash when viewing HTML on iOS

* [RNMobile] Remove toolbar from html view

* [RNMobile] Fix MaxListenersExceededWarning caused by dark-mode event emitter (#17186)

* Fix MaxListenersExceededWarning caused by dark-mode event emitter

* Checking for setMaxListeners trying to avoid CI error

* Adding remove listener to DarkMode HOC

* DarkMode: Binding this.onModeChanged to `this`

* DarkMode: Adding conditional needed to pass UI Tests on CI

* Fix focus title on new posts regression (#17180)

* BottomSheet: Setting DashIcon color directly when theme is default (light) (#17193)

* Activate Travis CI on rnmobile/master branch (#17229)

* Add native support for the MediaText block (#16305)

* First working version of the MediaText component for native mobile

* Fix adding a block to an innerblock list

* Disable mediaText on production

* MediaText native: improve editor visuals

* Move BlockToolbar from BlockList to Layout

* Remove BlockEditorProvider from BlockList and add native version of EditorProvider to Editor. Plus support InsertionPoint and BlockListAppender

* Update BlockMover for native to hide if locked or if it's the only block

* Make the vertical align button work, add more styling options for toolbar buttons

* Make sure registerCoreBlocks does not break in production

* Copy docblock comment from the web version for registerCoreBlocks

* Fix focusing on the media placeholder

* Only support adding image for now

* Update usage of MediaPlaceholder in MediaContainer

* Enable autoScroll for just the out most block list

* Fix JS Unit tests

* Roll back to IconButton refactor and fix tests

* Fix BlockVerticalAlignmentToolbar buttons style on mobile

* Fix thing for web and ensure ariaPressed is always passed down

* Use AriaPressed directly to style SVG on mobile

* Update snapshots

* MediaUpload and MediaPlaceholder unify props (#17145)

* Unify media placeholder and upload props within media-text (#17268)

* [RNMobile] Fix dismiss keyboard button for the post title (#17260)

* Recover border colors (#17269)

* [RNMobile] Insure tapping at end of post inserts at end

Previously, tapping at the end of the post would insert a block
immediately after the currently selected block. In addition, this commit
is cleaning out a few unusued props in the block-list file.

* Support group block on mobile (#17251)

* First working version of the MediaText component for native mobile

* Fix adding a block to an innerblock list

* Disable mediaText on production

* MediaText native: improve editor visuals

* Move BlockToolbar from BlockList to Layout

* Remove BlockEditorProvider from BlockList and add native version of EditorProvider to Editor. Plus support InsertionPoint and BlockListAppender

* Update BlockMover for native to hide if locked or if it's the only block

* Make the vertical align button work, add more styling options for toolbar buttons

* Make sure registerCoreBlocks does not break in production

* Copy docblock comment from the web version for registerCoreBlocks

* Fix focusing on the media placeholder

* Only support adding image for now

* Update usage of MediaPlaceholder in MediaContainer

* Enable autoScroll for just the out most block list

* Fix JS Unit tests

* Roll back to IconButton refactor and fix tests

* Fix BlockVerticalAlignmentToolbar buttons style on mobile

* Fix thing for web and ensure ariaPressed is always passed down

* Use AriaPressed directly to style SVG on mobile

* Update snapshots

* Support group block on mobile

* Extend shouldShowInsertionPoint condition to be false when group is selected

* Code refactor

* Update package-lock

* Remove redundant bg color within button appender (#17325)

* [RNMobile] DarkMode improvements (#17309)

* Remove the need to import `useStyle` and pass the theme prop on every instance that `withStyle` is used

* Implement dark-mode refactor on all components

* Fix broken native tests

* Fix default block appender background color on DarkMode

* DarkMode: Make `useStyle` a class function

* [RNMobile] Add autosave to mobile apps (#17329)

* [RNMobile] Fix crash when adding separator

* Build: remove global install of latest npm since we want to use the paired node/npm version (#17134)

* Build: remove global install of latest npm since we want to use the paired node/npm version
* Also update travis to remove --latest-npm flag

* [RNMobile] Try dark mode (iOS) (#17067)

* Adding dark mode component implemented on list and list block

* Adding DarkMode handling to RichText, ToolBar and SafeArea

* Mobile: Using DarkMode as HOC

* iOS DarkMode: Modified colors on block list and block container

* iOS DarkMode: Improved Header Toolbar colors

* iOS DarkMode: Removing background from buttons

* iOS DarkMode warning and unsupported

* iOS DarkMode: MediaPlaceholder

* iOS DarkMode: BottomSheets

* iOS DarkMode: Inserter

* iOS DarkMode: DefaultBlockAppender

* iOS DarkMode: PostTite

* Update hardcoded colors with variables

* iOS DarkMode: Fix bottom-sheet cell value color

* iOS DarkMode: More - PageBreak - Add Block Here

* iOS DarkMode: Better text color

* iOS Darkmode: Code block

* iOS DarkMode: HTML View

* iOS DarkMode: Improve colors on SafeArea

* Fix toolbar not avoiding keyboard regression

* Fix native unit tests

* Fix gutenberg-mobile unit tests

* Adding RNDarkMode mocks

* RNMobile: Fix crash when viewing HTML on iOS

* [RNMobile] Remove toolbar from html view

* [RNMobile] Fix MaxListenersExceededWarning caused by dark-mode event emitter (#17186)

* Fix MaxListenersExceededWarning caused by dark-mode event emitter

* Checking for setMaxListeners trying to avoid CI error

* Adding remove listener to DarkMode HOC

* DarkMode: Binding this.onModeChanged to `this`

* DarkMode: Adding conditional needed to pass UI Tests on CI

* Fix focus title on new posts regression (#17180)

* BottomSheet: Setting DashIcon color directly when theme is default (light) (#17193)

* Add a preliminary version of the AutosaveMonitor for mobile that calls the "bridge" and asks the native side to save the content

* Add autosave mock function for tests

* Fix merge conflicts

* Fix lint

* Re-add autosave on mobile that was removed erroneously during import-merge from rnmobile/master

* Remove native variant of AutosaveMonitor and introduces changes at  editor store level

* Default to false for `isEditedPostAutosaveable` on mobile. There was a typo in the returing value on the previous commit.

* Make sure to consider edits to the Title when checking if auto-save is needed

* Fix lint

*  Add isAppender functionality on mobile (#17195)

* Add isAppender functionality on mobile

* refactor isAppender conditions

* Replace dropZoneUIOnly in favour of showMediaSelectionUI

* deprecate dropZoneUIOnly and add disableMediaSelection prop

* Update test

* Refactor tests and change prop name

* Remove redundant empty lines

* Refactor conditions inside MediaPlaceholder

* Update block-editor CHANGELOG

* Update packages/block-editor/CHANGELOG.md

Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>

* Autosave monitor - Make the mobile editor ping the native at each keystroke, since the deboucing logic is already well defined in the apps. (#17548)

* [RNMobile] Refactor Dark Mode HOC (#17552)

* [RNMobile] Refactor the Dark Mode HOC to fix naming antipatterns

* Fix lint errors

* Add .native.js suffix to usePreferredColorScheme

* Update usage of theme props renamed to preferredColorScheme

* Update usage of theme props renamed to preferredColorScheme

* Add missing heading levels to the UI (H4, H5, H6) (#17533)

* Fix lint issue (#17598)

* Fix list filter on paste for RN mobile. (#17550)

* Fix method for RN mobile.

* Use array.From instead of slice.

* Remove comment and use Array.from directly

* Convert from NodeList spreadable to Array.from

* Fix lint errors.

* Fix documentation examples to use Array.from

* Add empty line.

* [RNMobile] Move MediaUploadPorgress to its own component folder (#17392)

* Move MediaUploadPorgress to its own component folder (native)

* MediaUploadProgress - Fix import to code standards

* MediaUploadProgress readme

* Mobile - MediaUploadProgress README update

* Rnmobile/fix link editing on start (#17631)

* Don't try to clear links if text is clean.

* Commented LinkUI removal test when no URL.

* Don't try to remove link if we are at start of link and no actual selection is

* Simplify the code to check for active link

* Update index.native.js

# Conflicts:
#	packages/format-library/src/link/index.native.js

* Make sure that HTML that is sent is correctly updated. (#17710)

* Make sure that HTML that is sent is correctly updated.

* Fix issue in Android.

* Fix lint issues.

* Pass active state to HeadingLevelIcon in order to native start working (#17747)

* Fix typo this.iOS -> this.isIOS (#17763)

* Fix lint errors (#17808)

* Update packages/components/src/primitives/svg/index.native.js

Co-Authored-By: etoledom <etoledom@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo approved these changes

@etoledom etoledom Awaiting requested review from etoledom

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta jorgefilipecosta is a code owner

@Soean Soean Awaiting requested review from Soean Soean is a code owner

@talldan talldan Awaiting requested review from talldan talldan is a code owner

@youknowriad youknowriad Awaiting requested review from youknowriad youknowriad is a code owner

Assignees
No one assigned
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Package] Block library /packages/block-library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marecar3 @gziolo @daniloercoli @youknowriad