Skip to content
This repository has been archived by the owner on May 19, 2020. It is now read-only.

fix(ToolbarMethods) Selecting list/quote option while link popup is open crashes demo - #266 #268

Closed
wants to merge 4 commits into from
Closed

fix(ToolbarMethods) Selecting list/quote option while link popup is open crashes demo - #266 #268

wants to merge 4 commits into from

Conversation

98lenvi
Copy link
Contributor

@98lenvi 98lenvi commented Mar 7, 2020

Issue #266

Now the web app doesn't crash when list options are selected when the link popover is open

Changes

  • In ToolbarMethods.js
    • In transformParagraphToList method, I have added one more condition check, ie of the text selection is only a link, The behavior of the popover is that it only opens when the selection is only a link.
    • If the selection is only a link, then the usual conversion of the paragraph to list doesn't take place

Flags

  • Can be improved If I figure out a way to increase the selection area when the selection is only a list

Related Issues

@elit-altum
Copy link
Contributor

@shrey-sachdeva2000 weren't you working on this ?

@98lenvi
Copy link
Contributor Author

98lenvi commented Mar 7, 2020

@shrey-sachdeva2000 weren't you working on this ?

It looked like he just raised this issue, so I thought to work on it. Even if he started, I'm sorry about that. I think we can work together to find a fix for this.

@sachdeva-shrey
Copy link
Contributor

@shrey-sachdeva2000 weren't you working on this ?

It looked like he just raised this issue, so I thought to work on it. Even if he started, I'm sorry about that. I think we can work together to find a fix for this.

No worries, please go ahead if you've found a fix!

@jolanglinais
Copy link
Member

You've carried over commits from the branch in the PR here: #267

@jolanglinais jolanglinais linked an issue Mar 9, 2020 that may be closed by this pull request
@jolanglinais jolanglinais added the Type: Bug 🐛 Something isn't working label Mar 9, 2020
@98lenvi
Copy link
Contributor Author

98lenvi commented Mar 9, 2020

@irmerk, My bad. I've removed the unnecessary commits, Please have a look at it now!

@jolanglinais
Copy link
Member

Nice! It looks like this solves it except for the block quote button.

Copy link
Member

@DianaLease DianaLease left a comment

Choose a reason for hiding this comment

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

@irmerk - the block quote button seems to work on this when I try!

@DianaLease
Copy link
Member

@irmerk - the block quote button seems to work on this when I try!

Nevermind, I misunderstood the issue. It does not work for the block quote button

@98lenvi
Copy link
Contributor Author

98lenvi commented Mar 11, 2020

@irmerk @DianaLease, My bad. I forgot about block quotes, Please check it now, I have added the fix for block quotes as well.

@jolanglinais
Copy link
Member

@Michael-Grover do you mind also having a go at testing this?

@98lenvi could you clean up all the commits to be in the format we use in the DEVELOPERS guide and also remove [WIP]

@98lenvi
Copy link
Contributor Author

98lenvi commented Mar 12, 2020

@irmerk, really sorry about the commit messages, will fix it as soon as possible!

@98lenvi 98lenvi changed the title [WIP] bugfix(ToolbarMethods ) Selecting list/quote option while link popup is open crashes demo - #266 fix(ToolbarMethods) Selecting list/quote option while link popup is open crashes demo - #266 Mar 12, 2020
@98lenvi
Copy link
Contributor Author

98lenvi commented Mar 12, 2020

@irmerk, I've made the changes, Please check it whenever you are free.

@@ -194,9 +194,11 @@ export const transformListToBlockQuote = (editor, type, value) => {
*/
/* eslint no-unused-expressions: 0 */
export const transformPtoBQSwap = (editor, type) => {
isSelectionInput(editor.value, CONST.BLOCK_QUOTE)
console.log('hi')
Copy link
Member

Choose a reason for hiding this comment

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

Get rid of this little guy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oopsie

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The little guy is gone, Please check it now!

@Michael-Grover
Copy link

@Michael-Grover do you mind also having a go at testing this?

looks like it works to me

Signed-off-by: lenvi <lenvin@oykuapp.com>
Copy link
Member

@DianaLease DianaLease left a comment

Choose a reason for hiding this comment

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

This crashes if you have a link inside of a list item, open the link popup, and then click the list toolbar icon.

@98lenvi
Copy link
Contributor Author

98lenvi commented Mar 14, 2020

@DianaLease, Please check it now, I've added the bugfix at a common place.

@jolanglinais
Copy link
Member

This looks geat @98lenvi! This is a bit unusual, but we're implementing a development branch to keep master more stable and for actual releases. Could you open a new PR but open it to merge this branch to development instead of master? This looks done so we can just do some testing and get it merged.

I posted more information about this in slack and will be updating the CONTRIBUTING and DEVELOPERS guides soon.

@98lenvi
Copy link
Contributor Author

98lenvi commented Mar 17, 2020

Please refer to #298

@98lenvi 98lenvi closed this Mar 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Selecting list/quote option while link popup is open crashes demo
6 participants