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.

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

Fix clear formatting on headers #3005

Merged
merged 4 commits into from Mar 26, 2019
Merged

Conversation

joewalsh
Copy link
Contributor

As per discussion in weekly planning, this PR makes the clear formatting butting clear header markup when any part of a header is selected

@joewalsh joewalsh added the Dependent PR PR is dependent on another PR - merge dependent PR first and update branch before merging label Mar 26, 2019
@joewalsh joewalsh added this to the 6.2.1 milestone Mar 26, 2019
Copy link
Contributor

@montehurd montehurd left a comment

Choose a reason for hiding this comment

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

I’m on my phone so I can’t test, but that looks roughly like one of the ways I tried. Not too worried about edge case stuff though if I can add tests later ;) 👍

@joewalsh joewalsh removed the Dependent PR PR is dependent on another PR - merge dependent PR first and update branch before merging label Mar 26, 2019
@tonisevener
Copy link
Collaborator

tonisevener commented Mar 26, 2019

A couple of things I'm noticing -

Article "Diamonds Are Forever (novel)" - the first paragraph has remove markup disabled. Maybe related to that template embedded earlier in the line but formatting after the template should be removable.

Simulator Screen Shot - iPhone X - 2019-03-26 at 14 30 32

I see some characters removed when removing formatting. I tested on italics, bold, and links -
out

@joewalsh
Copy link
Contributor Author

@tonisevener the first issue I'm seeing on develop as well so I can file that as a follow-on issue. The second issue is definitely a regression caused by this PR and I've pushed a fix

@joewalsh
Copy link
Contributor Author

@tonisevener it looks like the first issue was caused by the template changes, so I reverted that and made disabling the clear formatting button in templates a follow on task: https://phabricator.wikimedia.org/T219321

@joewalsh joewalsh merged commit e567023 into develop Mar 26, 2019
@joewalsh joewalsh deleted the bug/clear_formatting_headers branch March 26, 2019 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants