-
Notifications
You must be signed in to change notification settings - Fork 92
MarkUnknown checkbox visible with TranslateDoc - Fix #146 #160
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
MarkUnknown checkbox visible with TranslateDoc - Fix #146 #160
Conversation
Corresponding changes would have to be made to |
Yes! This was my initial approach. However, it was getting a bit difficult to hide One solution I thought of is: Instead of
|
I would add a class to those elements instead and trigger that. |
I did the same :D . Thanks! |
I make use of |
@@ -191,9 +191,9 @@ | |||
<i class="fa fa-file-text"></i> <span data-text="Translate_Document">Translate a document</span> | |||
</button> | |||
</div> | |||
<div class="col-lg-3 col-md-6 pull-right"> | |||
<div class="col-lg-3 col-md-6 pull-right" id="translateOptionsContainer"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I thought about this a bit and I'm not actually a huge fan of the siblings solution. It's clever and is probably the minimal diff or something but it requires carefully inspecting the HTML to figure out what things are siblings, etc. I would prefer a more explicit approach, e.g.
hiding/showing #translateText *:not(#translateOptionsContainer), #translateOptions *:not(#markUnknownContainer)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I would get on with this approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -147,6 +147,9 @@ if(modeEnabled('translation')) { | |||
if($('div#translateText').is(':visible')) { | |||
translateText(); | |||
} | |||
else if($('div#docTranslation').is(':visible')) { | |||
translateDoc(); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Um, I am not sure why this is appearing in the diff here :/ . I simply did a git rebase
with master. Do you suggest making a separate PR? (The below snippet has the changes that you asked to do @sushain97 )!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, maybe you're rebasing with your master instead of origin/master
? I think these changes are separate so we don't want to merge them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Um, I did a git fetch origin, git rebase origin/master
. I am not sure why it still exists. Is it okay if I create a new PR?
@@ -147,6 +147,9 @@ if(modeEnabled('translation')) { | |||
if($('div#translateText').is(':visible')) { | |||
translateText(); | |||
} | |||
else if($('div#docTranslation').is(':visible')) { | |||
translateDoc(); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, maybe you're rebasing with your master instead of origin/master
? I think these changes are separate so we don't want to merge them.
Closing here and continuing on #180 |
markUnknown
checkbox is visible to the user wherein the user can tick/untick the checkbox to achieve document translation. A different ID is used for checkbox intranslateDoc
div so that it can be independently mapped to the XHR data.fixes #146