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.

Sign up for GitHub

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

Jump to bottom

Disable duplicate button without selected profile #12096

Merged
3 commits merged into from
Jan 6, 2022
Merged

Disable duplicate button without selected profile #12096

3 commits merged into from
Jan 6, 2022

Conversation

davidegiacometti
Copy link
Contributor

@davidegiacometti davidegiacometti commented Jan 5, 2022 •
edited
Loading

Summary of the Pull Request

  • Settings > Add a new profile
  • Disable "Duplicate" button until a profile is selected

Duplicate

References

PR Checklist

  • Should take care of [Terminal]: 'Duplicate' button should remain in disabled state until user don't select a profile radio control. #12056, once we can get a build to the a11y team (MAINTAINER EDIT: we unfortunately can't just say "closes #foo" for issues like this one, we need another team to validate the following build.)
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated.
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@ghost ghost added Area-Accessibility Issues related to accessibility Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Jan 5, 2022
zadjii-msft
zadjii-msft approved these changes Jan 5, 2022
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Wow that was fast, thanks!

@zadjii-msft
Copy link
Member

@msftbot make sure @carlos-zamora signs off on this

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 5, 2022
@ghost
Copy link

ghost commented Jan 5, 2022

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I'll only merge this pull request if it's approved by @carlos-zamora

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 5, 2022
@carlos-zamora
Copy link
Member

Thanks for doing this!

@htcfreek
Copy link

htcfreek commented Jan 6, 2022

Small thought: I think it makes more sense to pre select the first profile by default or even better use a drop down menu.

@carlos-zamora
Copy link
Member

Small thought: I think it makes more sense to pre select the first profile by default or even better use a drop down menu.

Frankly, I'd prefer to replace all of our radio buttons with combo boxes simply because of the accessibility issues we've had with radio buttons (i.e.: can't update value using keyboard). Plus, combo boxes let you type a letter in and jump to the first relevant entry. But I think guidance is to not to for most of our use cases.

For this particular case though, I could go either way. We want every profile to have "equal" value, so radio buttons make sense. Also, the page would look pretty empty without the radio buttons haha. But if we added more stuff like "import profile (JSON)", that would solve that problem. And the accessibility issues I mentioned earlier make me prefer combo boxes. So yeah, idk. 🤷

@carlos-zamora carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 6, 2022
@ghost
Copy link

ghost commented Jan 6, 2022

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit a766798 into microsoft:main Jan 6, 2022
@DHowett DHowett added the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Jan 21, 2022
DHowett pushed a commit that referenced this pull request Jan 21, 2022
@davidegiacometti @DHowett
Disable duplicate button without selected profile (#12096)
b30137b 
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request

- Settings > Add a new profile
- Disable "Duplicate" button until a profile is selected

![Duplicate](https://user-images.githubusercontent.com/25966642/148303450-a084cd5f-7f1c-4de3-86bd-602b9336649e.gif)

<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? -->
## References

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Should take care of #12056, once we can get a build to the a11y team (MAINTAINER EDIT: we unfortunately can't just say "closes #foo" for issues like this one, we need another team to validate the following build.)
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed

(cherry picked from commit a766798)
@DHowett DHowett removed the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Jan 26, 2022
@ghost
Copy link

ghost commented Feb 3, 2022

🎉Windows Terminal Preview v1.13.10336.0 has been released which incorporates this pull request.:tada:

Handy links:

@zadjii-msft zadjii-msft mentioned this pull request Feb 6, 2022
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

Assignees

@carlos-zamora carlos-zamora

Labels
Area-Accessibility Issues related to accessibility AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@davidegiacometti @zadjii-msft @carlos-zamora @htcfreek @DHowett