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.

Sign up for GitHub

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

Jump to bottom

Added text direction feature #438

Merged
merged 5 commits into from
Oct 30, 2021
Merged

Added text direction feature #438

merged 5 commits into from
Oct 30, 2021

Conversation

Amir-P
Copy link
Collaborator

@Amir-P Amir-P commented Nov 16, 2020 •
edited
Loading

I've added text direction support to the both Notus and Zefyr. It matches the behavior of Quill's rich text editor based on my tests.

@Amir-P Amir-P changed the title Added text direction support Added text direction feature Nov 16, 2020
@Amir-P Amir-P force-pushed the feature/text_direction branch from e75604f to f1a6fa0 Compare September 11, 2021 12:12
@Amir-P Amir-P requested a review from pulyaevskiy September 11, 2021 12:12
@Amir-P
Copy link
Collaborator Author

Amir-P commented Sep 23, 2021 •
edited
Loading

I just removed LTR attribute since missing RTL attribute will be interpreted as LTR. I also removed using editor's parents directionality as default direction for text lines since it would cause getting different results using the same Notus document with different locales. @pulyaevskiy

@Amir-P Amir-P force-pushed the feature/text_direction branch from 766df57 to 21b1c3a Compare September 23, 2021 19:03
@Amir-P Amir-P mentioned this pull request Oct 21, 2021
Closed
@Amir-P Amir-P force-pushed the feature/text_direction branch from 21b1c3a to 6685ecc Compare October 21, 2021 07:17
@Amir-P
Copy link
Collaborator Author

Amir-P commented Oct 21, 2021

Rebased @pulyaevskiy .

pulyaevskiy
pulyaevskiy reviewed Oct 23, 2021
View reviewed changes
Copy link
Contributor

@pulyaevskiy pulyaevskiy left a comment

Choose a reason for hiding this comment

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

To be honest I really know nothing about LTR/RTL so I'm mostly asking questions about the topic in hopes to better understand it. PTAL at the comments.

packages/notus/lib/src/document/attributes.dart Show resolved Hide resolved
pulyaevskiy
pulyaevskiy reviewed Oct 24, 2021
View reviewed changes
Copy link
Contributor

@pulyaevskiy pulyaevskiy left a comment

Choose a reason for hiding this comment

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

Have a few more nits, but otherwise looks good. PTAL!

Thanks again for taking time to educate me on RTL.

packages/zefyr/lib/src/rendering/editable_text_block.dart Show resolved Hide resolved
packages/zefyr/lib/src/widgets/editable_text_block.dart
Comment on lines +69 to +59
children.add(Directionality(
textDirection: nodeTextDirection,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just me learning RTL again. I suspect we still need to wrap with Directionality so that everything else inside the EditableTextLine can pick it up correctly instead of relying on the default value inherited from the app?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Isn't that happening here? The EditableTextLine is wrapped with Directionality widget. I think I didn't understand what you said correctly. Can you explain more?

Copy link
Contributor

Choose a reason for hiding this comment

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

It was more of a question to confirm my understanding here. It just seems redundant to pass direction to the line widget but also wrap it with Directionality.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm now that I checked it again I can't remember why I did that. I think there was a reason for that back then but it makes no sense now. So I removed all of them and using provider pattern instead. PTAL.

packages/zefyr/lib/src/widgets/editable_text_block.dart Outdated
@@ -49,30 +49,43 @@ class EditableTextBlock extends StatelessWidget {
);
}

TextDirection getTextDirectionForNode(BuildContext context, StyledNode node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

BuildContext doesn't seem to be used here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was being used before for getting default directionality but it isn't needed anymore.

packages/zefyr/lib/src/widgets/editor.dart Outdated
return TextDirection.rtl;
}
return TextDirection.ltr;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This method seems to be duplicated in a couple of places. Would be nice to have it in one place only.

@Amir-P Amir-P force-pushed the feature/text_direction branch from 1f8f25d to da206bd Compare October 25, 2021 07:12
@Amir-P Amir-P requested a review from pulyaevskiy October 25, 2021 07:14
pulyaevskiy
pulyaevskiy approved these changes Oct 30, 2021
View reviewed changes
Copy link
Contributor

@pulyaevskiy pulyaevskiy left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@pulyaevskiy pulyaevskiy merged commit 43b3755 into memspace:1.0-dev Oct 30, 2021
@pulyaevskiy pulyaevskiy deleted the feature/text_direction branch October 30, 2021 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pulyaevskiy pulyaevskiy pulyaevskiy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Amir-P @pulyaevskiy