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

Shortcut exit link 2393 #124

Merged
merged 4 commits into from Jul 3, 2023

Conversation

vincenzoursano
Copy link
Contributor

  • Press ESC to exit the link editing mode without saving the entered text if any.
  • Press ESC again to close up the link menu.

AppFlowy-IO/AppFlowy#2393

@annieappflowy

Support the mobile platforms (iOS and Android) and make the editor more extensible.
@CLAassistant
Copy link

CLAassistant commented May 16, 2023

CLA assistant check
All committers have signed the CLA.

Comment on lines 149 to 154
onKey: (key) {
if (key is! RawKeyDownEvent) return;
if (key.logicalKey == LogicalKeyboardKey.escape) {
widget.onDismiss();
}
},
Copy link
Collaborator

@Xazin Xazin May 17, 2023

Choose a reason for hiding this comment

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

Sort child last (below this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -21,6 +21,7 @@ void main() async {
onSubmitted: (text) {
submittedText = text;
},
onDismiss: () {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a test case for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 454 to 456
onDismiss: () {
_dismissOverlay();
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
onDismiss: () {
_dismissOverlay();
},
onDismiss: _dismissOverlay,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return RawKeyboardListener(
focusNode: FocusNode(),
onKey: (key) {
if (key is! RawKeyDownEvent) return;

Choose a reason for hiding this comment

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

Suggested change
if (key is! RawKeyDownEvent) return;
if (key is! RawKeyDownEvent) {
return;
}

IIRC, Effective Dart says to use curly braces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

controller: _textEditingController,
onSubmitted: widget.onSubmitted,
decoration: InputDecoration(
hintText: 'URL',

Choose a reason for hiding this comment

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

Please use a translatable string for this hint text.

https://appflowy.gitbook.io/docs/essential-documentation/contribute-to-appflowy/software-contributions/translation

See locale_keys.g.dart... you can add a key, value pair of your choice to en.json and run a task (control+shift+p, then type task: generate...) to generate the LocaleKeys, then you can use it here like so:

hintText: LocaleKeys.url_label.tr();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, within appflowy-editor, I don't understand how the language management works, and there is a lack of documentation. It's okay to add it to the main project, but I don't understand how to pass it to appflowy-editor.
Can we do it in a separate pull request so that we can manage all the labels?

Choose a reason for hiding this comment

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

Let's add internationalization before this pull request lands. You can DM me on discord (a-wallen#0603) if you need help. I'm happy to hop on a call to show you how this is done.

cc @LucasXu0, we might want to add better documentation for internationalization per this comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure. The translation for the editor is different from the main project. Here's the guide link: https://github.com/AppFlowy-IO/appflowy-editor/blob/main/documentation/translation.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @LucasXu0 , done!

),
border: const OutlineInputBorder(
borderRadius: BorderRadius.all(Radius.circular(12.0)),
borderSide: BorderSide(color: Color(0xFFBDBDBD)),

Choose a reason for hiding this comment

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

Please reference a grey color from the app theme. See color_scheme.dart. The shaders from color_scheme should be approximate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :)

Copy link

@a-wallen a-wallen left a comment

Choose a reason for hiding this comment

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

Good work so far. Please address my comments at your convenience :)

@LucasXu0
Copy link
Collaborator

LucasXu0 commented May 22, 2023

Hey, @vincenzoursano. Thanks for contributing. Would you mind changing the merge branch to the mobile?

@vincenzoursano vincenzoursano changed the base branch from main to mobile May 22, 2023 21:34
@vincenzoursano
Copy link
Contributor Author

Hi @LucasXu0, I changed the merge branch. Do we need to do anything else?

@annieappflowy
Copy link
Collaborator

Status?

@a-wallen
Copy link

a-wallen commented Jun 5, 2023

LGTM @annieappflowy, but cc @LucasXu0 because there are no CI workflows running tests.

@vincenzoursano
Copy link
Contributor Author

Sorry, I fixed the commit message that was blocking the CI.

@LucasXu0 LucasXu0 changed the base branch from mobile to main July 3, 2023 08:30
@codecov
Copy link

codecov bot commented Jul 3, 2023

Codecov Report

Merging #124 (bcadc15) into main (8bfb37f) will increase coverage by 0.03%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##             main     #124      +/-   ##
==========================================
+ Coverage   62.61%   62.64%   +0.03%     
==========================================
  Files         246      246              
  Lines       10857    10869      +12     
==========================================
+ Hits         6798     6809      +11     
- Misses       4059     4060       +1     
Impacted Files Coverage Δ
.../toolbar/desktop/items/link/link_toolbar_item.dart 87.75% <ø> (ø)
lib/src/service/selection_service.dart 0.38% <0.00%> (-0.01%) ⬇️
...c/editor/toolbar/desktop/items/link/link_menu.dart 100.00% <100.00%> (ø)
lib/src/l10n/intl/messages_en.dart 98.66% <100.00%> (+0.01%) ⬆️
lib/src/l10n/l10n.dart 46.28% <100.00%> (+0.67%) ⬆️

@LucasXu0 LucasXu0 merged commit c8082d3 into AppFlowy-IO:main Jul 3, 2023
7 checks passed
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.

None yet

6 participants