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

[ZEPPELIN-1757] Menu of paragraph includes keyboard shortcut #1736

Closed
wants to merge 10 commits into from

Conversation

soralee
Copy link
Contributor

@soralee soralee commented Dec 8, 2016

What is this PR for?

Currently, keyboard shortcuts can be only seen by keyboard icon.
It'll be very comfortable if keyboard shortcuts are showing the menu of paragraph.

What type of PR is it?

[Improvement]

Todos

  • Add the keyboard shortcut of Link this paragraph
  • Update CI(Selelium) Test case

What is the Jira issue?

How should this be tested?

Click to paragraph menu. I tested on Ubuntu.

Screenshots (if appropriate)

[before]
screenshot from 2016-12-08 14-52-35

[after]
image

z1736

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@astroshim
Copy link
Contributor

@soralee Thank you for contribution of useful function.
but I am usually using the Shift + Enter to run paragraph,
How can be presented this shortcut?
and it seems necessary a shortcut for the Link this paragraph. What do you think?

@soralee
Copy link
Contributor Author

soralee commented Dec 9, 2016

@astroshim Thanks for suggestion.
The shortcut key of Run this paragraph is being presented by tooltip already.
image

BTW, I agree the shortcut key of Link this paragraph seems to be necessary. I'll improve shortcut key and will fix CI in order to be green light.

@soralee soralee closed this Dec 13, 2016
@soralee soralee reopened this Dec 13, 2016
@soralee soralee closed this Dec 14, 2016
@soralee soralee reopened this Dec 14, 2016
@AhyoungRyu
Copy link
Contributor

@soralee Regarding ParagraphActionsIT.java change, IMO it would be better to be handled by another PR so that we can only focus on the scope of this patch. There are already many issues related ParagraphActionsIT.java as Flaky Test category(see here). If this change that you made can fix the problem then we can merge it first and then you can rebase based on the latest master.

@soralee
Copy link
Contributor Author

soralee commented Dec 14, 2016

@AhyoungRyu I appreciate to your information!
I think I already fixed ParagraphActionsIT.java file. But, each time I tested the CI, the build operation failed differently. So I just wondered that.
Let me do the rebase and test again.

@soralee
Copy link
Contributor Author

soralee commented Dec 14, 2016

Regarding Link this paragraph shortcut key, I just created this issue. (#1756)
If that issue (#1756) merged, Let me update this feature.

@soralee soralee force-pushed the ZEPPELIN-1757 branch 2 times, most recently from c7031c9 to 3cdcde9 Compare December 20, 2016 05:01
@soralee
Copy link
Contributor Author

soralee commented Dec 20, 2016

I updated description of PR as like below.

  • add shortcut key to tooltip (cancel to run paragraph, toggle editor, toggle output)
  • add shortcut key to Link this paragraph in paragraph menu

@soralee soralee force-pushed the ZEPPELIN-1757 branch 4 times, most recently from a5ce623 to 3ce66e7 Compare December 20, 2016 07:42
@soralee
Copy link
Contributor Author

soralee commented Dec 20, 2016

Ping Ping ! Ci is green.
It's time to ready to review this PR 😄

@Leemoonsoo
Copy link
Member

This is screenshot i got from this branch.

image

I'm not sure why some items are missing and text are not aligned. @soralee Could you check?

@soralee
Copy link
Contributor Author

soralee commented Dec 27, 2016

Sure! Let me check and test again 😃
Thanks for test @Leemoonsoo

@soralee soralee force-pushed the ZEPPELIN-1757 branch 2 times, most recently from fae8121 to 5b84f23 Compare December 27, 2016 08:27
@soralee soralee closed this Dec 27, 2016
@soralee soralee reopened this Dec 27, 2016
@soralee
Copy link
Contributor Author

soralee commented Dec 27, 2016

@Leemoonsoo I fixed the problem regarding not aligned text.
Could you test and check again, please?

@Leemoonsoo
Copy link
Member

Tested again. And still not every items are displayed

image

@soralee could you check?

@soralee
Copy link
Contributor Author

soralee commented Dec 28, 2016

Sorry, I've missed your review @Leemoonsoo
If your note has only one paragraph and you checked that menu of paragraph, it normally works like your screenshot. Because that paragraph doesn't need some function like Move up and Move down and Remove.
If you want to see all item, could you check middle paragraph menu after making three paragraph?

@Leemoonsoo
Copy link
Member

@soralee you're right. I checked again and it looks good to me.
Thanks for the improvement and merge if there're no further comments!

@asfgit asfgit closed this in e3cc8ea Dec 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants