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

Mispositioning toolbar #294

Merged
merged 7 commits into from
Feb 10, 2020
Merged

Conversation

wdtamagi
Copy link
Contributor

@wdtamagi wdtamagi commented Jan 4, 2020

fix for #291

This PR fix toolbar position for highest left and right.

@wdtamagi
Copy link
Contributor Author

wdtamagi commented Jan 5, 2020

I don't understand the issues of Codacy...

@wdtamagi
Copy link
Contributor Author

wdtamagi commented Jan 5, 2020

The Codacy rule is without semi and the eslint.prettier/prettier rule is with semi...

Copy link
Contributor

@mogavin mogavin left a comment

Choose a reason for hiding this comment

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

An arrow spacing is occurring when the prop movableBlocks from MegadraftEditor is false. Could you please check?

image

@wdtamagi
Copy link
Contributor Author

wdtamagi commented Jan 7, 2020

@mogavin Sure i will work on this fix, thanks for your reply.
And about Codacy issues?

@mogavin
Copy link
Contributor

mogavin commented Jan 7, 2020

Thanks, @wdtamagi ! We will also check Codacy issues

@wdtamagi wdtamagi requested a review from mogavin January 8, 2020 00:44
@wdtamagi
Copy link
Contributor Author

wdtamagi commented Jan 8, 2020

@mogavin Can you check again?

Copy link
Contributor

@mogavin mogavin left a comment

Choose a reason for hiding this comment

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

The feature looks fine now, but one of the tests is not passing ( make test command )

image

P.S: We still analysing de Codacy issue 👍

@wdtamagi
Copy link
Contributor Author

wdtamagi commented Jan 9, 2020

Ops, sorry about that.
I will work on this.
Thanks @mogavin

@mogavin
Copy link
Contributor

mogavin commented Jan 9, 2020

@wdtamagi, Codacy is ok now 👍

@wdtamagi
Copy link
Contributor Author

@mogavin
I fixed the test, but I don't understand the method "replaceSelection" not really replace the mocked selection...

@wdtamagi wdtamagi requested a review from mogavin January 11, 2020 22:03
@mogavin
Copy link
Contributor

mogavin commented Jan 13, 2020

Thanks, @wdtamagi. As for replaceSelection method, I didn't understand your question. Could you detail it better?

@wdtamagi
Copy link
Contributor Author

wdtamagi commented Jan 13, 2020

In the toolbar test have a method:

const replaceSelection = (newSelection, wrapper) => {
  const selectionState = SelectionState.createEmpty("ag6qs");
  const updatedSelection = selectionState.merge(newSelection);
  const oldState = wrapper.state("editorState");

  const editorState = EditorState.forceSelection(oldState, updatedSelection);

  wrapper.setState({ editorState: editorState });
};

This method mock the editorState of component, but all the math for calculate the position of toolbar use de DOM elements (src/utils.js method getSelectionCoords).

I believe for a really useful test of positioning of toolbar need mock the DOM position of editor for make things happen.

I can try do this on this PR or in another one.

@mogavin
Copy link
Contributor

mogavin commented Jan 14, 2020

@wdtamagi
I believe the objective of method replaceSelection is to change the selectionState portion of editorState, which influences the return of method getVisibleSelectionRect (used in getSelectionCoords). But any suggestions for improvement will be very welcome, thanks for your contribution!

@mogavin mogavin changed the base branch from master to new-release February 10, 2020 18:34
@mogavin mogavin merged commit 57964d5 into globocom:new-release Feb 10, 2020
@KyleLawson16
Copy link
Contributor

This seems to have stopped the toolbar from going out of view of the window, however now I'm seeing an issue with positioning of the toolbar with selected text on the left side of the editor. Running v0.7.0

megadraft-toolbar

@mogavin
Copy link
Contributor

mogavin commented Mar 11, 2020

Hi @KyleLawson16 ! Yep, we have become aware of this problem recently. We will soon work on a correction ( but, as always, we are open to contributions too 😀 ). Thanks !

@KyleLawson16
Copy link
Contributor

Hey @mogavin I was able to find some time to tackle it #310

@mogavin
Copy link
Contributor

mogavin commented Mar 11, 2020

Great, @KyleLawson16! We will review this PR and provide feedback soon. Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants