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-1747] Fix Korean notename input problem #1723

Closed
wants to merge 2 commits into from

Conversation

marchpig
Copy link
Contributor

@marchpig marchpig commented Dec 3, 2016

What is this PR for?

Korean notename is incorrectly typed on Firefox.
This PR fixes the issue by changing placeholder attribute of the input field.
Getting the placeholder text from noteName() is unnecessary because the text is visible only when the notename is blank.

What type of PR is it?

Bug Fix

Todos

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-1747

How should this be tested?

Type Korean notename in action bar using Firefox browser.

Screenshots (if appropriate)

korean-notename-issue

Questions:

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

@tae-jun
Copy link
Contributor

tae-jun commented Dec 3, 2016

I tested on Firefox and the error is gone. LGTM 👍

image

And I guess the CI failure is unrelated. Could you re-trigger CI?

@marchpig marchpig closed this Dec 3, 2016
@marchpig marchpig reopened this Dec 3, 2016
@marchpig
Copy link
Contributor Author

marchpig commented Dec 3, 2016

@tae-jun Thanks for your quick reply.
I triggered CI again.

@Leemoonsoo
Copy link
Member

Thanks @marchpig for the fix.
Thanks @tae-jun for the review.

LGTM and merge to master if there're no more comments.

@@ -15,7 +15,7 @@
<h3>
<div style="float: left; width: auto; max-width: 40%"
ng-controller="ElasticInputCtrl as input">
<input type="text" pu-elastic-input class="form-control2" placeholder="{{noteName(note)}}" style="min-width: 0px; max-width: 95%;"
<input type="text" pu-elastic-input class="form-control2" placeholder="Note {{note.id}}" style="min-width: 0px; max-width: 95%;"
Copy link
Contributor

Choose a reason for hiding this comment

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

I understood this fix.
But displaying a note id in the placeholder seems to be weird also, isn't it?
The placeholder is just a simple value or a short description of the expected input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cuspymd Thanks for your comment! As you can see, I just wanted to fix the Korean input problem without any changes to the existing behavior. However, I'm not sure what value would be appropriate for this placeholder.

Copy link
Contributor

Choose a reason for hiding this comment

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

@marchpig A watch is not cheap resource in AnguarJS.
Let's erase meaningless interpolation '{{..}}'.
How about just "New Name" ?

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 @cuspymd for your suggestion. "New name" seems to be appropriate!

@Leemoonsoo
Copy link
Member

Thanks @cuspymd for providing review.
LGTM and merge to master if there're no more comments.

@asfgit asfgit closed this in 83320c7 Dec 6, 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