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

2nd Fix for Placeholder on Heading block #753

Merged
merged 1 commit into from
Mar 18, 2019

Conversation

daniloercoli
Copy link
Contributor

This PR is a second tentative on fixing #707 and uses much of the code introduced in #739 but doesn't relies on eventCount , instead it actually checks the previous text if it's empty or not, before wrapping the newly inserted text with default tag.

cc @marecar3

To test this PR

Test 1

  • Add a new heading block
  • The placeholder text should appear
  • Start typing in the block
  • The styling should be there and the toolbar updated

Test 2

  • Remove all the text in a Heading block
  • The placeholder text should appear again
  • Start typing in the block
  • The styling should be there and the toolbar updated

Test 3

  • Add a new heading block
  • The placeholder text should appear
  • Paste a text in the block
  • The styling should be there and the toolbar updated

Make sure to test with Heading block added at the end of the main list.

@marecar3
Copy link
Contributor

Hey, @daniloercoli I have found a bug on Paragraph block.

Steps :

  • Create a new paragraph
  • Write some text
  • Select all as bold
  • Move to the beginning of the text
  • Start typing new text

Result -> Every other letter will be bold.

every_other_letter_bold

@daniloercoli
Copy link
Contributor Author

Is it related to these changes @marecar3? Would you mind to test develop, since I guess it's the default behavior of Aztec.

I tried the steps above on a old version of the editor, and can reproduce it, the toolbar button B is not highlighted in your picture though, instead here is highlighted.

Steps I tried:

  • Create a new paragraph
  • Write some text
  • Select all with the context menu
  • Apply Bold
  • Move to the beginning of the text
  • Start typing new text

@marecar3
Copy link
Contributor

Hey @daniloercoli, I couldn't reproduce it on develop branch.

@daniloercoli
Copy link
Contributor Author

Maybe i didn't get the testing steps to reproduce the problem, but this is what I see on develop when applying bold on a para block, and then start typing at the beginning:
selection-android

@daniloercoli
Copy link
Contributor Author

daniloercoli commented Mar 18, 2019

It seems that there were already problems on develop in regards the formatting bar. After a chat convo I made 2 videos to show other variations of the initial problem reported by @marecar3.
Pasting here the link to 2 videos I made running develop on a testing device of mine. https://cloudup.com/ct7LMVYR4J0

I guess we can open a new issue, and move forward this PR. To test it again, just put a breakpoint inside the if condition here and see that it's not hit at all during the testing steps reported above.

Copy link
Contributor

@marecar3 marecar3 left a comment

Choose a reason for hiding this comment

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

Hey @daniloercoli , thanks for the confirmation, LGTM! Let's move forward.

Please, if you can open the new issue for problem that we have found on the develop? Thanks.

@daniloercoli daniloercoli merged commit 914e398 into develop Mar 18, 2019
@daniloercoli daniloercoli deleted the issue/707-Placeholder-missing-take-2 branch March 18, 2019 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants