Skip to content

Feature: Added a description to each IAction #11828

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

Merged
merged 32 commits into from
Apr 16, 2023

Conversation

hishitetsu
Copy link
Member

@hishitetsu hishitetsu commented Mar 24, 2023

Resolved / Related Issues

  • Were these changes approved in an issue or discussion with the project maintainers? In order to prevent extra work, feature requests and changes to the codebase must be approved before the pull request will be reviewed. This prevents extra work for the contributors and maintainers.
    Closes Feature: Add a description to each IAction #11786

Validation
How did you test these changes?

  • Did you build the app and test your changes?
  • Did you check for accessibility? You can use Accessibility Insights for this.
  • Did you remove any strings from the en-us resource file?
    • Did you search the solution to see if the string is still being used?
  • Did you implement any design changes to an existing feature?
    • Was this change approved?
  • Are there any other steps that were used to validate these changes?

@yaira2
Copy link
Member

yaira2 commented Mar 24, 2023

Once this is finished, we can port it over the to the website repo.

@hishitetsu
Copy link
Member Author

I filled in all the descriptions.

@QuaintMako
Copy link
Contributor

@hishitetsu I've added a few commands I forgot in #11481.

@hishitetsu
Copy link
Member Author

@hishitetsu I've added a few commands I forgot in #11481.

Thank you! But for now, this list only covers those merged into main. Once this has been merged, it is ideal to update this document along with the code when submitting a PR.

@QuaintMako
Copy link
Contributor

@hishitetsu I've added a few commands I forgot in #11481.

Thank you! But for now, this list only covers those merged into main. Once this has been merged, it is ideal to update this document along with the code when submitting a PR.

We will need to open our eyes for PRs then and make sure we do!

@hishitetsu
Copy link
Member Author

Don't worry! If the list is missing an update, I will maintain it.
However, it might be a good idea to add a check to the PR template.

@yaira2
Copy link
Member

yaira2 commented Mar 26, 2023

Should we leave this open until we finish with the rich command work?

@hishitetsu
Copy link
Member Author

That's also possible. I hope to merge this by the time we start implementing the command palette or shortcut customization.

Copy link
Member

@0x5bfa 0x5bfa left a comment

Choose a reason for hiding this comment

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

Please don't add spaces in table.

@hishitetsu
Copy link
Member Author

hishitetsu commented Mar 28, 2023

Please don't add spaces in table.

You mean | Category | should be | Category | for example?

@0x5bfa
Copy link
Member

0x5bfa commented Mar 28, 2023

Yes. and separator should be --- each

@hishitetsu
Copy link
Member Author

At first I did so, but @yaira2 put in the space. I think it is easier to see it as it is now, but why shouldn't it include spaces?

@0x5bfa
Copy link
Member

0x5bfa commented Mar 28, 2023

@yaira2 This kind indentation will be meaningless even if it is readability by following two major problems.

  • When you have to add a new text that has more letters than expected.
  • When you have to edit a existing text, you have to adjust spacing.

I also thought this is nice idea before, but it's tiresome, annoying.

@yaira2
Copy link
Member

yaira2 commented Mar 28, 2023

I used a tool to format the table, spaces or dashes are fine, but it doesn't really matter since there are many tools that can format it for you.

@yaira2
Copy link
Member

yaira2 commented Apr 14, 2023

@hishitetsu do you want to add the descriptions to the resource file? I think we have enough to move forward with this.

@hishitetsu
Copy link
Member Author

@hishitetsu do you want to add the descriptions to the resource file? I think we have enough to move forward with this.

Ok, I will do it within a few days.

@hishitetsu hishitetsu changed the title Docs: Added Command list Feature: Added a description to each IAction Apr 15, 2023
@hishitetsu
Copy link
Member Author

Ok, I will do it within a few days.

Done.

Copy link
Member

@yaira2 yaira2 left a comment

Choose a reason for hiding this comment

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

LGTM

@yaira2 yaira2 added the ready to merge Pull requests that are approved and ready to merge label Apr 16, 2023
@yaira2 yaira2 merged commit 507592d into files-community:main Apr 16, 2023
@hishitetsu hishitetsu deleted the CommandList branch April 16, 2023 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Add a description to each IAction
4 participants