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

💡 [Feature]: Remove or Refactor the show/hide tenant wide extension list #325

Open
Adam-it opened this issue Oct 15, 2024 · 13 comments · May be fixed by #409
Open

💡 [Feature]: Remove or Refactor the show/hide tenant wide extension list #325

Adam-it opened this issue Oct 15, 2024 · 13 comments · May be fixed by #409
Assignees
Labels
⭐ enhancement New feature or request 💪 good first issue Good for newcomers 👨‍💻work in progress I am working on it
Milestone

Comments

@Adam-it
Copy link
Member

Adam-it commented Oct 15, 2024

🎯 Aim of the feature

Since now the list of extensions only loads when the node is expanded to ensure that the additional API calls don't affect the extension load time there is little sense in having the setting that allows to show/hide the extension list. We could consider the following:

  • removing this setting. This may seem a breaking change and maybe we should consider doing it with next major v5 release which should also bring multiple connections. This release is a bit blocked due to ESM support (different issue/story 😛)
  • refactoring this setting to something more global. It could now allow to either show(load)/hide apps from all app catalogs. So when set to true all app catalogs will be expandable and will show the app list. When set to true we would only show app catalogs without the possibility to show apps

📷 Images (if possible) with expected result

The issue relates to the following setting:
image

🤔 Additional remarks or comments

Related issue #215

Tagging @Saurabh7019 as initial author of this idea 👍. You Rock 🤩

Prototype

  1. we should rename this setting and correct its description

    vscode-viva/package.json

    Lines 187 to 191 in 5919011

    "spfx-toolkit.showTenantWideExtensions": {
    "title": "Show tenant-wide extensions",
    "type": "boolean",
    "default": true,
    "description": "Show the tenant-wide extensions in the account view."

    it should be something like Show apps in app catalogs and description like When set it will load all apps in every app catalog on your tenant
  2. we should remove this
    const showTenantWideExtensions: boolean = getExtensionSettings<boolean>('showTenantWideExtensions', true);
    and just always load the tenant wide extensions
  3. we should use the new setting here
    const tenantAppCatalogNode = new ActionTreeItem(tenantAppCatalogUrl.replace(origin, '...'), '', { name: 'globe', custom: false }, TreeItemCollapsibleState.Collapsed, 'vscode.open', `${Uri.parse(tenantAppCatalogUrl)}/AppCatalog`, 'sp-app-catalog-url', undefined,
    and it similar place in site app catalogs so that when this setting is set to false we should not make it expandable and not add the expand handle
  4. this setting should be true by default
@Adam-it Adam-it added this to the v4.X milestone Oct 15, 2024
@nicodecleyre
Copy link
Contributor

Hi, @Adam-it, just wondering. You define in the last bullet point 2 scenarios 'when set to true'. I was wondering if it shouldn't be:

  • When set to true, all app catalogs will be expandable and display the app lists.
  • When set to false, only the catalogs themselves will be visible, without the option to expand and view individual apps.

@Adam-it
Copy link
Member Author

Adam-it commented Oct 27, 2024

Hi, @Adam-it, just wondering. You define in the last bullet point 2 scenarios 'when set to true'. I was wondering if it shouldn't be:

  • When set to true, all app catalogs will be expandable and display the app lists.
  • When set to false, only the catalogs themselves will be visible, without the option to expand and view individual apps.

Yes exactly that is what I had in mind 👍. Thank you for clarifying

@Adam-it
Copy link
Member Author

Adam-it commented Nov 17, 2024

Ok let's get this shipped and not wait for a major release, this settings don't make much sense anyway.
I will create some prototype to guide how to go about it and then let's open it up

@Adam-it
Copy link
Member Author

Adam-it commented Nov 18, 2024

I added a prtotype with guidance how to go about it in the orginal issue post. Let's open it up for others 🚀

@ervingayle
Copy link
Contributor

Hi @Adam-it . I would like to try this one.

@Adam-it Adam-it added 👨‍💻work in progress I am working on it and removed 🙏 help wanted Open for contributors labels Feb 1, 2025
@Adam-it
Copy link
Member Author

Adam-it commented Feb 1, 2025

Hi @Adam-it . I would like to try this one.

all yours 👍

@ervingayle
Copy link
Contributor

Hi @Adam-it . I have these changes ready. I am having an issue submitting a pull request for just these changes due to differences in the branches and my prior custom steps pull request. :)

@Adam-it
Copy link
Member Author

Adam-it commented Feb 3, 2025

Hi @Adam-it . I have these changes ready. I am having an issue submitting a pull request for just these changes due to differences in the branches and my prior custom steps pull request. :)

Why? Did you use the same branch to add those changes as the branch for custom steps?
Can you sen me over the link to you branch so that I may have a look?

@ervingayle
Copy link
Contributor

Yes, I used the same branch. Its possible that I am doing something wrong.
Here is the link

@Adam-it
Copy link
Member Author

Adam-it commented Feb 3, 2025

Yes, I used the same branch. Its possible that I am doing something wrong. Here is the link

I see. Well it depends how you look at it, but in general, when using git for each feature/issue you code you should create a separate branch based on the branch you plan to merge.
So for example if you plan to create new feature for the dev branch you may checkout it out locally, then based on it create a new branch like featureA, then commit all your changes to that branch and after you are done push it to your fork of this repo and open a PR from your featureA branch to target dev branch from this repo.

In the next post I will prepare a small solution how you may fix this situation

@Adam-it
Copy link
Member Author

Adam-it commented Feb 3, 2025

In order to fix it up you may do the following:
first, open the terminal and go to your repo (folder location).
To be sure you are on dev branch with all your work run:

git switch dev
git pull

now run git log to check the history, you will need to lookup the hash of your commits.
now lets create a fresh new dev branch based on the commit which is aligned with dev from this repo (so you will have a branch with the same history as dev branch in this repo). In order to do that lets create a branch dev2 based on this commit . In order to do that run

git branch dev2 883ea0abcdab6e70ee81779913d29feb71cd0ae7

you may run git branch to verify the list of branches and that your new branch is there. Now lets switch to it running git switch dev2.
Now you will have a fresh new dev2 branch which is aligned with dev from this repo.
Now lets create a new branch for your feature from this issue. to do that lets run

git checkout -b feature/issue-325

you may pick better name if you want. Now you will have a new branch to which you may add your changes, commit them and push them and then open a PR. Since you already did your work there is no need to recreate it from scratch or copy paste it. We may use the cherry-pick command to copy a commit from one branch to other. I assume you did your changes in this commit. So now to move it lets run

git cherry-pick d70edf4305031b0ec5f5f1631cd746d3832142a2

If you need to move more commits simply rerun this command with hash of those commits. And that's it. You should have a new feature/issue-325 branch with your changes based on dev2 branch which is aligned with dev branch from this repo.
You may verify using the git log command if all your work is on that branch.
Now it's time to run git push if this will be first push for this branch you will need to run git push --set-upstream feature/issue-325. And after that just go to your repo on GitHub and open a PR that targets dev branch from this repo.

I hope the above will be helpful.
Let me know if it go you unblocked.

@ervingayle
Copy link
Contributor

Hi @Adam-it . I really appreciate you taking the tie to write this up. I typcally follow this approach of creating a new branch as I did for the Hackathon projects but I took the easy way out in this case. I did some quick research as well to see how I could salvage my first commit and then cherry-pick the changes in the 325 item. In that branch, I tried
git reset --hard HEAD^ (since this is the most recent push to my branch)
git push --force
Now I have this.
Do you think that I can address your suggested changes to address the currently open PR?

@Adam-it
Copy link
Member Author

Adam-it commented Feb 4, 2025

yes it seems to be possible. I see you created a new branch 325 which is a copy of your dev so no what you could do is fixup your dev branch to reset it to be the same as dev from this repo.
Then we may close this PR without merging #404 as it makes little sense now.
Then create new branches for those two issues you are assigned to based on your fixed dev
And then cherry-pick only the commits you need for each of those issues to each branch from your 325 branch
So how I would go about it.

  1. close PR Add possibility to add custom additional step #404 for now and we will get back to it when we clean up the git history problem
  2. then open up your terminal in the location of your vscode viva repo fork
  3. now lets create a backup branch for your dev to make sure we don't loose any code you wrote. To do that switch to dev by running git switch dev. Now lets create backup branch git checkout -b dev-backup
  4. now go to GitHub to your forked repo of vscode-viva and overwritte your dev to align it 1:1 with dev from this repo. To do that you may use the sync fork button

Image
Most probably in your case you won't have a green Update branch button but some red Discard button but don't worry. This is what we want, We want to fixup dev to align with pnp:dev and you have a backup done locally.

  1. now go back to terminal and lets remove your local dev branch and checkout the dev from GitHub again so that you will have your changes aligned with pnp:dev. Switch to dev-backup running git switch dev-backup, don't worry we wont delete the backup we just need to be on some different branch than dev in order to delete it. Now lets remove the dev by running git branch -D dev. Now checkout dev from GitHub running git checkout dev. You should now have a local dev branch which is exactly the same as pnp:dev. You may check that running git log and comparing the history
  2. now create a new branch for your issue for development like for example for issue 325 like git checkout -b feature/issue-325
  3. now cherry pick to this new branch your commits from dev-backup. Cherry pick only the commits that were needed for issue 💡 [Feature]: Remove or Refactor the show/hide tenant wide extension list #325. So run git cherry-pick .... you will need to lookup the commit hash yourself by switching to dev-backup and running git log and there you will see the commit hash.
  4. after that run git push to push it to GitHub and then go to GitHub and open a PR that targets dev branch.
  5. ... do the same for the issue 💡 [Feature]: Add possibility to add custom additional step #217

let me know if it helped. If needed we may do all that over a short call but no sooner than next week. This week my calendar is fully booked already

@ervingayle ervingayle linked a pull request Feb 7, 2025 that will close this issue
3 tasks
@Adam-it Adam-it linked a pull request Feb 15, 2025 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⭐ enhancement New feature or request 💪 good first issue Good for newcomers 👨‍💻work in progress I am working on it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants