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

Add save button to settings UI #7949

Merged
merged 6 commits into from
Oct 16, 2020
Merged

Add save button to settings UI #7949

merged 6 commits into from
Oct 16, 2020

Conversation

cinnamon-msft
Copy link
Contributor

@cinnamon-msft cinnamon-msft commented Oct 16, 2020 •
edited by carlos-zamora
Loading

Added save and reset buttons.
Added warning message for unsaved settings.

save/reset buttons and warning

References

#1564 - Settings UI Epic

@cinnamon-msft @carlos-zamora
Add settings UI string localization (#7833)
8b402c5 
<!-- 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)? -->
Moved all strings into resources file for localization.

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

<!-- Please review the items on the PR checklist before submitting-->
* [ ] Closes #xxx
* [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.
* [x] 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 -->

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
@cinnamon-msft
add save button
eda0d20
@DHowett
Copy link
Member

DHowett commented Oct 16, 2020

@leonMSFT or @carlos-zamora, please merge master into settings-ui before we review this PR.

@DHowett
Copy link
Member

DHowett commented Oct 16, 2020 •
edited
Loading

(this branch is based off .. something weird, but merging into feature/settings-ui. thus the conflicts.)

@carlos-zamora
Copy link
Member

(this branch is based off .. something weird, but merging into feature/settings-ui. thus the conflicts.)

Kayla was having trouble building the project on her machine (too big). So I did some git magic to put her branch in a state where the Settings UI was its own app. Updated feature/settings-ui and merged that in. This PR is now ready for review!

@carlos-zamora
Copy link
Member

NOTE: the warning message is currently always displayed. Once we set up data binding, we need to make it so that when a setting is changed, the warning is visible (otherwise, hidden).

@carlos-zamora carlos-zamora added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Oct 16, 2020
leonMSFT
leonMSFT reviewed Oct 16, 2020
View reviewed changes
Copy link
Contributor

@leonMSFT leonMSFT left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is in response to Carlos's comment - should we also gray-out the Save button if there's nothing to save, also maybe gray-out the reset button if there's nothing changed? (perhaps is a nice little touch but also a bit unnecessary 🤔)

Also I'm curious how the warning TextBlock and the Buttons arrange themselves when you compress the window to a much smaller size, could I get a pic of that?

src/cascadia/TerminalSettingsEditor/MainPage.xaml
<ColumnDefinition Width="*"></ColumnDefinition>
<ColumnDefinition Width="200"></ColumnDefinition>
</Grid.ColumnDefinitions>
<TextBlock x:Uid="Settings_UnsavedSettingsWarning" Grid.Column="0" Foreground="Goldenrod" FontSize="15" VerticalAlignment="Center" HorizontalAlignment="Left" TextAlignment="Left" Margin="30,0,0,0"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need tooltips for these controls (TextBlock, buttons)?

Copy link
Member

Choose a reason for hiding this comment

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

I dunno. The text block is its own description.

Should we simply hide the entire save/reset/warning panel when there's nothing to show?

Copy link
Contributor

Choose a reason for hiding this comment

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

😆 I now realize asking about a text block tooltip is uh a little redundant, I think the reset button could benefit from a tooltip clarification though (I can see myself thinking it would "reset to default settings"), or maybe it could be called "Discard Changes"?

Copy link
Contributor Author

@cinnamon-msft cinnamon-msft Oct 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We tried mockups with "Discard Changes" and it looked funny. I hope the tooltip I added is good enough?

@cinnamon-msft
Copy link
Contributor Author

This is in response to Carlos's comment - should we also gray-out the Save button if there's nothing to save, also maybe gray-out the reset button if there's nothing changed? (perhaps is a nice little touch but also a bit unnecessary 🤔)

Also I'm curious how the warning TextBlock and the Buttons arrange themselves when you compress the window to a much smaller size, could I get a pic of that?

I agree with the greying out comment, but I figured that would be done programmatically? Unless you want me to have it start that way?

Also here's what it looks like at its min width:
image

@DHowett
Copy link
Member

DHowett commented Oct 16, 2020

yeah we should gray them out or hide them in code. no need for you to do it in this PR 😄

leonMSFT
leonMSFT approved these changes Oct 16, 2020
View reviewed changes
Copy link
Contributor

@leonMSFT leonMSFT left a comment

Choose a reason for hiding this comment

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

👍 🚢

@carlos-zamora carlos-zamora merged commit c14824e into feature/settings-ui Oct 16, 2020
@carlos-zamora carlos-zamora deleted the cinnamon/save-button branch October 16, 2020 23:14
DHowett
DHowett reviewed Oct 16, 2020
View reviewed changes
src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw
@@ -651,10 +651,14 @@
<data name="Settings_ResetSettingsButton.Content" xml:space="preserve">
<value>Reset</value>
</data>
<data name="Settings_ResetSettingsButton.[using:Windows.UI.Xaml.Controls]ToolTipService.ToolTip" xml:space="preserve">
<value>Reset your settings to your previous save.</value>
Copy link
Member

Choose a reason for hiding this comment

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

Lol this phrasing sounds like a video game.
Discard may be better :p

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

@DHowett DHowett DHowett left review comments

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

@leonMSFT leonMSFT leonMSFT approved these changes

Assignees
No one assigned
Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cinnamon-msft @DHowett @carlos-zamora @leonMSFT