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

select text on comment open #4349

Merged
merged 12 commits into from Oct 12, 2020

Conversation

halitanildonmez
Copy link
Contributor

The basics

  • [ X] I branched from develop
  • [ X] My pull request is against develop
  • [ X] My code follows the style guide

The details

Resolves

#1863

Proposed Changes

Selects the text inside a comment box when it is opened

Reason for Changes

To solve the issue at hand

Test Coverage

For each platform I have used tests/playground.html and created a function to open comment

Tested on:
Desktop Chrome
Desktop Firefox

Documentation

No it was not, though I would be willing to update it if need be.

Additional Information

@alschmiedt alschmiedt self-requested a review October 6, 2020 18:12
@alschmiedt
Copy link
Contributor

I think the original issue has to do with workspace comments instead of block comments. You can create a workspace comment by right clicking on the workspace and selecting "Add Comment" from the context menu.

If you are still interested in working on this issue I think looking in workspace_comment_svg.js would be a good starting place. I would also mention, that it should only highlight the workspace comment if it has the default text.

Thanks!

@halitanildonmez
Copy link
Contributor Author

Wow I have misunderstood this one 😄

Going to fix it asap! Thanks for the input!

@halitanildonmez
Copy link
Contributor Author

done! 😄

Comment on lines 167 to 169
if (this.textarea_) {
this.textarea_.select();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is on the right track!

My only concern is that this initSvg method will get called in fromXml which will cause all of the comments to be selected when someone loads a workspace from xml. This can be tested by going to the playground, creating a workspace comment, changing the text, and then hitting the "Export to XML" and the "Import from XML" buttons.

I think this can be fixed if we add an optional parameter to this method that can be passed in as true in fromXml. We can then check that the optional parameter is false before selecting the area.

Thanks! And let me know if you have any questions!

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 for the help!

I am not sure about the final status: :

I created a new boolean parameter deSelectText if it is false I am selecting the text
From fromXml method I set the flag to true.

It feels a bit strange to me, especially with the variable name, let me know what you think!

again thanks for the help!

Copy link
Contributor

@alschmiedt alschmiedt left a comment

Choose a reason for hiding this comment

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

It looks like a few build files got added that need to be removed.

Other than that lgtm once the comments are addressed : )

core/workspace_comment_svg.js Outdated Show resolved Hide resolved
core/workspace_comment_svg.js Outdated Show resolved Hide resolved
@halitanildonmez
Copy link
Contributor Author

It should be fixed now

@halitanildonmez
Copy link
Contributor Author

Hmm looks like the build failed due to a missing file /home/travis/build/google/blockly/blockly_compressed.js but that is generated with the build right ?

I deleted it since it was not intended to be committed.

Copy link
Contributor

@alschmiedt alschmiedt left a comment

Choose a reason for hiding this comment

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

The files that were accidentally added shouldn't be deleted, they should just be reverted to their previous state. You should be able to do something like:
git checkout <commit> filename.
The only change for the pr should be in workspace_comment_svg.js.

Thanks for the updates! I know getting the compressed files wrapped up in a pr can be a pain : ) I have definitely done it a few times.

@@ -145,9 +145,12 @@ Blockly.WorkspaceCommentSvg.prototype.dispose = function() {
/**
* Create and initialize the SVG representation of a workspace comment.
* May be called more than once.
*
* @param {boolean=} deSelectText Text inside text area will be selected if false
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment needs to be updated to use opt_noSelect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok now things looks better 😄

@alschmiedt
Copy link
Contributor

This looks good, but there is still one file that needs to be reverted msg/js/ja.js.

Copy link
Contributor

@alschmiedt alschmiedt 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 @halitanildonmez!

@alschmiedt alschmiedt merged commit c913c02 into google:develop Oct 12, 2020
@alschmiedt alschmiedt modified the milestone: 2020_q4_release Oct 12, 2020
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

2 participants