Added tour response for Upgrade and Validate actions, Closes #188 #323
Conversation
nicodecleyre
changed the title
|
Awesome. Thank you for another awesome contribution. I will review it ASAP |
There was a problem hiding this comment.
@nicodecleyre Awesome work so far 👏👏👏👏
I tested it and works really good. During my tests and review I noticed some small details we could sort out before we merge 👍
Please do give them a double check 🙏
|
@nicodecleyre I also did some merging to the dev branch. May I kindly ask you to rebase against latest dev |
|
@nicodecleyre we just rebased |
nicodecleyre
force-pushed
the
Tour-response-for-Upgrade-and-Validate-#188
branch
3 times, most recently
from
aa49dd8
to
67811be
Compare
Still working on it, having a little trouble with the rebase 😆 |
nicodecleyre
force-pushed
the
Tour-response-for-Upgrade-and-Validate-#188
branch
from
67811be
to
f36347f
Compare
There was a problem hiding this comment.
@nicodecleyre I think we are in a much better state now. I played with it a lot and I think it is a nice feature. I noticed some two small improvements we could double check before we proceed but those are not that important I guess.
What is my main concern, and this is more a question if you experienced the same behavior, is:
when I had both for validation and upgrade I noticed that sometimes when clicked on the upgrade action even though the tour was generated and the json was present te codeTour extension did not start it but instead it asked which tour should it run and none was available? I was wondering if the tour extension triggered just before the tour json was present 🤔 Maybe we could add a bit of wait as I checked this is already awaited so I am not sure why this behavior and I would need a bit deeper dive on this myself
And then the strange part. When I had the validation tour created first and then I clicked on the upgrade action the extension kicked of the validation tour 😮 instead of asking which tour should be triggered?
I was wondering did you maybe researched if there was a way to pass down to codeTour which specific tour should start, so we could avoid the picker when we have both the validation and upgrade tours? I will try to research this myself as well.
Other than the above concerns/questions I think we are all good and getting closer to get this merged 👍👍I wanted to make this feature perfect before we proceed 🤩. I will try to recheck some of the stuff mentioned above myself next week
| if (projectUpgradeOutputMode === 'markdown') { | ||
| resultMd = await CliExecuter.execute('spfx project upgrade', 'md'); | ||
| CliActions.handleMarkdownResult(resultMd, wsFolder, 'upgrade'); | ||
| } else if (projectUpgradeOutputMode === 'code tour') { | ||
| await CliExecuter.execute('spfx project upgrade', 'tour'); | ||
| CliActions.handleTourResult(wsFolder, 'upgrade'); | ||
| } else { | ||
| resultMd = await CliExecuter.execute('spfx project upgrade', 'md'); | ||
| await CliExecuter.execute('spfx project upgrade', 'tour'); |
There was a problem hiding this comment.
I was wondering if we could just have if blocks in which we check for either the specific type/method, like markdown or tour and check for both. In this case we don't need to check explicitly for both and we will avoid code repetition. What do you think? I guess it should work
| if (projectUpgradeOutputMode === 'markdown') { | |
| resultMd = await CliExecuter.execute('spfx project upgrade', 'md'); | |
| CliActions.handleMarkdownResult(resultMd, wsFolder, 'upgrade'); | |
| } else if (projectUpgradeOutputMode === 'code tour') { | |
| await CliExecuter.execute('spfx project upgrade', 'tour'); | |
| CliActions.handleTourResult(wsFolder, 'upgrade'); | |
| } else { | |
| resultMd = await CliExecuter.execute('spfx project upgrade', 'md'); | |
| await CliExecuter.execute('spfx project upgrade', 'tour'); | |
| if (projectUpgradeOutputMode === 'markdown' || projectUpgradeOutputMode === 'both') { | |
| resultMd = await CliExecuter.execute('spfx project upgrade', 'md'); | |
| CliActions.handleMarkdownResult(resultMd, wsFolder, 'upgrade'); | |
| } | |
| if (projectUpgradeOutputMode === 'code tour' || projectUpgradeOutputMode === 'both') { | |
| await CliExecuter.execute('spfx project upgrade', 'tour'); | |
| CliActions.handleTourResult(wsFolder, 'upgrade'); | |
| }``` |
| if (fs.existsSync(tourFilePath)) { | ||
| await commands.executeCommand('codetour.startTour'); | ||
| } else { | ||
| Notifications.error('Validation tour file not found. Cannot start Code Tour.'); |
There was a problem hiding this comment.
the error may be misleading as it may not always be the validation tour but also it may be the upgrade tour
|
Hi Adam, you're right, there seems to be a sort of hickup. It seems that the setup right now "still lives in the past". I did some testing and this are the results:
So it seems that only the tour from the previous run is selectable. Timeout doesn't seem to solve the problem right away. Will have to dig deeper. |
I wonder if we could open a issue in the code tour extension repo to ask for some advice 🤔? |
🎯 Aim
Adding the tour options and both md & tour options as settings for the upgrade and validate actions
📷 Result
✅ What was done
Md,Tour,Bothas upgrade options in the settingsMd,Tour,Bothas validate options in the settings🔗 Related issue
Closes: #188