-
Notifications
You must be signed in to change notification settings - Fork 94
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
Issue #734: Context menu & menu bar edits #957
Conversation
Codecov Report
@@ Coverage Diff @@
## master #957 +/- ##
==========================================
- Coverage 92.87% 92.23% -0.64%
==========================================
Files 50 50
Lines 5288 4922 -366
Branches 1022 873 -149
==========================================
- Hits 4911 4540 -371
- Misses 375 380 +5
Partials 2 2
Continue to review full report at Codecov.
|
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.
Looks awesome! 😋
Thanks for cleaning things up! 👍
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.
11fd8ed
Fixed this for Copy & Edit Data Set/Member |
I just added it in the latest version :) So handled now, yes. |
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.
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.
Thanks @katelynienaber for the updates in this PR!
Apologies for being slow on this review. It took me some time to get what was happening with the changes to DS createFile and USS createUSSNode and why, but I understand now.
Overall, the PR looks good! All unit and expected integration tests are passing. I think it is near ready for approval, but I have some quick suggestions and questions that I think should be addressed before this goes into master branch (see code references).
I know you are focusing heavily on the base profile PR now, so I tried to figure out some of the issues to make polishing this PR quicker and easier for you. Hope it helps!
@zFernand0 @katelynienaber Just a heads-up when bringing this branch up-to-date with the master branch: The context menu commands for favoriting a USS session and favoriting a Jobs session are now Therefore, you may encounter a merge conflict in package.json that looks like: It should be resolved to look something like below. (Note especially the
|
Aaand again XD @lauren-li I've added this back. |
Just a short start of a page here: https://github.com/zowe/vscode-extension-for-zowe/wiki/Working-with-Context-Menus-and-Icons-in-Zowe-Explorer |
I went about clicking on the commands involved in this PR, and I found a bit of odd behavior when trying to favorite a profile/search in the Jobs view. Clicking "Add to Favorites" doesn't seem to add it into the Job view's Favorites section. However, when I next click on the profile/search, it will act like a profile/search item in the Favorites section, and show the context menu for a favorited profile/search, even though it is not located in the Favorites section. The resulting error is probably due to it not being under the Favorites section, and might go away once we map the command correctly. I also think we may want to reorganize the commands in |
Signed-off-by: katelynienaber <katelyn.nienaber@broadcom.com>
@lauren-li I've fixed the dropdown issue, please check again ^^ |
@katelynienaber Thank you for addressing the issue I found. I don't see it happening anymore. While manually retesting, I noticed a couple other issues. (Hopefully these will be the last ones!):
I think this PR is getting really close to being good to go. We are just at the polishing stage now. Thanks Katelyn for all your hard work on this! |
Signed-off-by: katelynienaber <katelyn.nienaber@broadcom.com>
@lauren-li This is like whack-a-mole XD Ok please give it another shot |
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.
Thanks @katelynienaber!
I made some suggestions for the when... viewItem =~
values for USS Copy Path, USS Rename, and USS Pull from Mainframe, as it seems those commands got removed from some context menus where they should have remained.
I like the addition of the inline "Remove Search" icons for favorited searches. Although I didn't ask for them since we never had them before, I did always think it was a bit odd we excluded them. The new icons seem to function well. I made some suggestions for the USS "Remove Search" icon to make it more consistent with MVS and Jobs.
I went ahead and made PR #1153 into your PR with these requested changes since I know your plate is already full with like 3 other pull requests that are a bit more complicated. Feel free to bring #1153 into this PR if you agree with the suggestions. I hope this helps, and that we can get this PR merged into master
soon!
@lauren-li Thanks for reviewing again! I added these changes to this branch, but I would prefer not to merge your branch in. I noticed some weird behavior on #1153, ex. clicking a favorited search in USS adds an extra profile node, and the Remove Favorites icons are missing on the inline bar. Maybe I was going too fast and built it wrong somehow. But these things are working on this branch so I added your changes here and would prefer to just merge this one (#957) without #1153. Let me know what you think! |
@katelynienaber I believe this is an existing issue (#1029 USS Favorited search creates extra profile node).
Oh no, sorry. That is somehow missing from my PR. Thanks for catching that. Since you brought in the changes from PR #1153, I will go ahead and close it. I'll retest this PR (#957) and let you know how it goes! |
@katelynienaber I think this PR is almost there! I promise I'm not finding these issues one-at-a-time on purpose. All the issues I mentioned above are fixed, but I just noticed for sequential data sets only, the "Copy" and "Paste" commands are missing (for both favorited and non-favorited sequential data sets). The expected commands show fine for PDS's and members. The below screenshot is for a sequential data set: Other than that, I did not notice any other issues. I counted the commands and compared them to the expected numbers for each item type this time, so I really hope this is the last bit of polish! All unit and expected integration tests are passing. |
@katelynienaber Actually, maybe we want to leave off the "Paste" command for sequential data sets. The current (v1.11.1) behavior is, if you "Copy" a data set member or other sequential data set, and then select "Paste" for a second sequential data set, the content of the second sequential data set gets completely replaced by the content of the copied data set. The mainframer I spoke with said this behavior does not seem intuitive, and I feel this is an easy way for users to accidentally lose data in sequential data sets. So perhaps, let's leave off the "Paste" command for sequential data sets (as you currently have in this PR), but re-add the "Copy" command for sequential data sets, if @jelaplan is ok with that? |
We can discuss it in standup. The function exists in master and this PR is only about rearranging the context menus, not deciding whether any functions are useful or intuitive to users. But if everyone agrees that it is a useless function then I can remove it after standup so that we don't have to open another PR. |
Re-add Copy for sequential data sets (but leave out Paste, which you have taken out). |
Signed-off-by: katelynienaber <katelyn.nienaber@broadcom.com>
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.
everything looks good! thank you @katelynienaber
Signed-off-by: katelynienaber <katelyn.nienaber@broadcom.com>
@lauren-li Just pushed, check again pls ^^ |
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.
@katelynienaber Thank you for all your hard work, patience, and perseverance on this PR. I think this looks good now!
Proposed changes
Small changes to context menus & menu bars according to conversation with @jelaplan and the team:
@lauren-li I tagged you for review because you had helped with reordering the context menus before :)
Release Notes
Milestone: Backlog
Changelog: All commands moved/copied into the context menus, and some commands renamed to be more specific.
Types of changes
What types of changes does your code introduce to Zowe Explorer?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the reviewernpm run vscode:prepublish
has been executedFurther comments
This PR does NOT close Issue #734. It just happens to be related. So please don't close the issue! :D