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

Settings: small tweaks to VCM page #24735

Merged
merged 7 commits into from
Mar 21, 2023
Merged

Settings: small tweaks to VCM page #24735

merged 7 commits into from
Mar 21, 2023

Conversation

Jay-o-Way
Copy link
Collaborator

@Jay-o-Way Jay-o-Way commented Mar 10, 2023 •
edited
Loading

Summary of the Pull Request

  • removed ViewModel.IsVideoConferenceBuild
  • changed Visibility to IsEnabled for the Clear button
  • moved Visibility= to the SettingsCard

TO-DO:

PR Checklist

  • Closes: #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated
  • New binaries: Added on the required places
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Note
The current UI (visibility) in 0.68.1 for the image settings does not behave as expected:
image

Validation Steps Performed

Visual Studio?

@Jay-o-Way Jay-o-Way changed the title Settings: Remove VCM logic Settings: Remove IsVideoConferenceBuild Mar 10, 2023
@Jay-o-Way Jay-o-Way marked this pull request as ready for review March 16, 2023 15:55
@Jay-o-Way Jay-o-Way added the Needs-Review This Pull Request awaits the review of a maintainer. label Mar 16, 2023
@stefansjfw stefansjfw requested a review from yuyoyuppe March 17, 2023 20:30
@Jay-o-Way
Copy link
Collaborator Author

Jay-o-Way commented Mar 18, 2023 •
edited
Loading

  1. Don't know how many pc's are running N edition - without the pack - but should we mention this, somewhere?
  2. There are quite a number of rather small things mentioned in [Settings] UI suggestions #18866. Is there anything I can add to this pr, or should I spread things over many small pr's?

@Jay-o-Way Jay-o-Way changed the title Settings: Remove IsVideoConferenceBuild Settings: small tweaks to VCM page Mar 18, 2023
@Jay-o-Way Jay-o-Way requested review from Aaron-Junker and removed request for niels9001 March 18, 2023 13:02
@Aaron-Junker Aaron-Junker dismissed their stale review March 19, 2023 12:33

Feedback adressed

yuyoyuppe
yuyoyuppe approved these changes Mar 20, 2023
View reviewed changes
Copy link
Contributor

@yuyoyuppe yuyoyuppe left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!

@yuyoyuppe
Copy link
Contributor

@Jay-o-Way let's split those fixes into different PRs, since it'd be easier to review.

should be good when debuggin, right

make sure that the working dir is correct.

Jay-o-Way
Jay-o-Way commented Mar 20, 2023
View reviewed changes
src/settings-ui/Settings.UI/Views/VideoConference.xaml
</StackPanel>
</labs:SettingsCard>
<labs:SettingsCard>
<labs:SettingsCard
Visibility="{Binding Path=CameraImageOverlayPath, Mode=OneWay, Converter={StaticResource EmptyToCollapsedConverter}}">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@niels9001 When I change EmptyToCollapsedConverter to StringVisibilityConverter, things break. Do you know why?

@stefansjfw
Copy link
Collaborator

@Jay-o-Way what else is needed here? Do you want to try to update the converter or ok to merge it as it is?

@Jay-o-Way
Copy link
Collaborator Author

@stefansjfw Yeah, I suspect the cause/solution is simple, might be a broken reference or something? If it can be fixed that easily, this would be done :)

stefansjfw
stefansjfw approved these changes Mar 21, 2023
View reviewed changes
Copy link
Collaborator

@stefansjfw stefansjfw left a comment

Choose a reason for hiding this comment

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

Nice polish. Thanks!

@stefansjfw stefansjfw merged commit 061d724 into microsoft:main Mar 21, 2023
@Jay-o-Way Jay-o-Way deleted the remove-VCM-logic branch March 21, 2023 12:07
BLM16 pushed a commit to BLM16/PowerToys that referenced this pull request Jun 22, 2023
@Jay-o-Way @BLM16
Settings: small tweaks to VCM page (microsoft#24735)
26ba856 
* Remove `IsVideoConferenceBuild`

* small tweaks VCM page

* Button: Visibility --> IsEnabled

* Revert "Remove `IsVideoConferenceBuild`"

This reverts commit acae5a5.

* negate IsOpen bool
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htcfreek htcfreek htcfreek requested changes

@stefansjfw stefansjfw stefansjfw approved these changes

@yuyoyuppe yuyoyuppe yuyoyuppe approved these changes

@Aaron-Junker Aaron-Junker Awaiting requested review from Aaron-Junker

Assignees
No one assigned
Labels
Needs-Review This Pull Request awaits the review of a maintainer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Jay-o-Way @yuyoyuppe @stefansjfw @Aaron-Junker @htcfreek