-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Add underline to format toolbar on the RN version. #17483
Conversation
This branch seems to include a lot more than just the underline right? I highly discourage adding an underline button. It doesn't add any semantic meaning, it's just styling. In addition, underlines are usually used for links, so it might confuse readers of the web page. We removed the underline button from WordPress a long time ago, and we've never added it it Gutenberg either. |
@ellatrix it looks I targeted the wrong branch. On mobile we are using an older version of master. I've now updated the PR to correct branch. Regarding the underline I still see it on WP.com atomic sites. It's not on the top level of the toolbar but you can find it in the drop down options. |
For reference here is the issue I was solving on GB-mobile |
* [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
…e is selected. This does map the format library value `core/underline` ---> `undeline` and pass it to native.
@ellatrix I feel like I remember seeing a lengthy conversation around underline and whether it should be added at all on the web side. It does appear that it is in the toolbar, tucked in a dropdown menu. I was under the impression that it wasn't there until recently, so I imagine the aforementioned discussion led us to add it there. With that said, I think we should add it for parity (even though I agree with most of your rationale behind not having it on the toolbar). @SergioEstevao Can we add it to the end of the tool list? |
Neither Gutenberg nor WP has an underline button. Only a keyboard shortcut.
…On Wed, 25 Sep 2019 at 18:11, Thomas Bishop ***@***.***> wrote:
@ellatrix <https://github.com/ellatrix> I feel like I remember seeing a
lengthy conversation around underline and whether it should be added at all
on the web side. It does appear that it is in the toolbar, tucked in a
dropdown menu.
I was under the impression that it wasn't there until recently, so I
imagine the aforementioned discussion led us to add it there. With that
said, I think we should add it for parity (even though I agree with most of
your rationale behind *not* having it on the toolbar).
@SergioEstevao <https://github.com/SergioEstevao> Can we add it to the
*end* of the tool list?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17483?email_source=notifications&email_token=ABD6B22HTB4HY7SDCX4542LQLN5SJA5CNFSM4IYLK4H2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7SIJWA#issuecomment-535069912>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABD6B25TLS7O5DAMF5KKQUDQLN5SJANCNFSM4IYLK4HQ>
.
|
Ahh, ok, then this must just a WordPress.com thing. If we only supported self-hosted sites I would say remove it altogether, but since we support both self-hosted and atomic (and AFAIK, we can't toggle visibility based on type of site) I would defer to Aztec in this case (display |
I see the underline option on https://frontenberg.tomjn.com |
I think @ellatrix is the best person to review this PR given that it might enable the button in the web interface as well. |
@gziolo I think the underline is only added to .native side. Check this line: https://github.com/WordPress/gutenberg/pull/17483/files#diff-aa7b78d30bc35a328807700f2850e216R10 I'm only changing this on the .native file not in the web file. |
|
@gziolo if that is the case should I create a .native version of the underline file? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it's needed somehow for the mobile version, but we shouldn't add an underline button to the web version. I see this is currently the case as the format type is shared.
Are you adding it based on this comment? wordpress-mobile/gutenberg-mobile#562 (comment)
Gutenberg web doesn't have it... |
@@ -33,6 +34,15 @@ export const underline = { | |||
character="u" | |||
onUse={ onToggle } | |||
/> | |||
<RichTextToolbarButton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to explore whether we could use some sort of platform guard which would conditionally include this component only for mobile apps. Similar to what they do in react-native-web
:
https://github.com/necolas/react-native-web/blob/0e81c6ef2758d4ca9b2099b1d04a4f8c417f0f43/packages/react-native-web/src/exports/Platform/index.js
RN docs: https://facebook.github.io/react-native/docs/platform-specific-code#platform-module
I think @koke shared this idea some time ago. It would be a great opportunity to test it in action. I know that you use the native version directly from react-native
library. We could expose it from @wordpress/element
.
cc @youknowriad, @hypest and @Tug
@ellatrix we still see it on WP.com and while I know this repo is WP.org specific the mobile app needs to support both WP.com and WP.org sites. Even on WP.org if we have the possibility of adding underlines by shortcuts in the app we will need a way to enable/remove it too, because there is no shortcuts support there. If the current state of the PR changes the web interface too, I can change it to only activate on the mobile app. |
It seems Jetpack is doing it: https://github.com/Automattic/jetpack/blob/master/modules/wpcom-block-editor/class-jetpack-wpcom-block-editor.php#L295 |
I think this is something we need to figure out eventually in the mobile apps. There will be some things specific to WordPress.com/Jetpack that we want to tweak, but can't have in the core repo. I think in this case we should be able to do the same thing that Jetpack/Calypso do for this and register a custom format type. It's just a matter of figuring out where to put that code. As long as we still have the gutenberg-mobile repo, we can keep those "plugins" there, but we'll need a better solution when we move to a monorepo. |
I keep hitting reply too fast 😅
That is right, but it also works the other way around. If that is the reasoning for adding underline on mobile, we should also ensure that we're not showing the button on self hosted sites without Jetpack, or they will have the same problem when they try to edit their mobile posts on the web. |
@SergioEstevao I can't comment in /**
* External dependencies
*/
import { Platform } from 'react-native';
export default Platform; |
@koke I updated the PR, it looks my git tool "forgot" to see that change :) |
Hey there, coming to this a little cold but would love some context, excuse the questions as I do. I do feel that just putting something in for a mobile experience isn't going to be something preferred. If this is styling to be included in the actual web then we'll want to add in a design feedback label and get everyone commenting. What do you think @iamthomasbishop? For me, in this case it would be something to leave out. Now, I would love to know more specifically:
|
@karmatosed to be clear we don't want to add it to the web interface, the last code changes make sure it's only in the mobile. The initial idea of adding this to mobile was to have feature parity with Aztec, our current editor in mobile. |
@karmatosed Happy to give some more context, and hope to answer your questions 😄 To be clear, I don't think the intention was ever to add this to web – only native mobile. With that said, there's been a lot of discussion around this topic and there are a lot of layers to it, so it's a bit hard to explain succinctly. At the core of the challenge (no pun intended) is the fact that the editor(s) in the WordPress mobile apps are at the intersection of core, WordPress.com, and our current default editor Aztec (which includes an underline option). One of our initial goals with Mobile Gutenberg has been Aztec parity (Aztec includes underline, because it was based on the "classic" editors), so that was the idea behind this addition. We had intended to only include this for native mobile (to achieve Aztec parity), but in the end I think we (and @SergioEstevao @maxme @koke @hypest can maybe confirm/deny because I also missed some of the conversation) decided to not include a toolbar button for the time being, for a couple reasons: usage data is pretty low, and the semantic rationale which we understand to be valid. Like core, there are alternative ways to achieve this (on core/web that's a keyboard shortcut on core/web, and a selection menu shortcut on native), so that's also being taken into account. |
Thanks everyone for your comment there. Some of my thoughts:
Conclusion for this PR: we're going to close it and we won't have the underline feature in mobile gutenberg for now. There is also a non-technical solution to avoid disappointing our users: explain them why we made such changes, via an in-app help, FAQ or support. In the future, we want to create a way to package the mobile version integrating non community-features on top of the community project (this repo). We're not sure how we'll do this. It would let us add this specific Jetpack or WPCom features without making changes to this repo. Underline could be a the first of these features. |
@maxme I was thinking to just remove the button from the toolbar but keep the rest of the code on this PR, the new Platform component can be helpful for other PRs. |
@SergioEstevao if it's not too complicated, can you cherry pick commits related to the Platform component, open a new PR and close this one? |
@@ -0,0 +1,9 @@ | |||
const Platform = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 💯
Yes, please. It would help to minimize the number of full file overrides which will make maintenance a bit easier. |
Description
This PR adds the underline command to the default format options on the RN version.
How has this been tested?
Use this PR on mobile GB to test this functionality: wordpress-mobile/gutenberg-mobile#1370
Screenshots
Types of changes
Checklist: