Skip to content

Feature: Added a theming option to display a background image #15290

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
Merged
merged 6 commits into from
May 3, 2024
Merged

Feature: Added a theming option to display a background image #15290

merged 6 commits into from
May 3, 2024

Conversation

yaira2
Copy link
Member

@yaira2 yaira2 commented May 2, 2024 •
edited
Loading

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 support for setting a background image for the app #12636

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?
    1. Open settings file and assign path to an image file.
    2. Open Files and confirm the image is displayed.
    3. Adjust the different properties in the settings UI, and confirm the different combinations work properly.

ToDo

  • Add UI for selecting image
  • Add UI for removing existing image (ie. setting the path value to null)

Screenshots (optional)
image
image
image

@yaira2 yaira2 changed the title Feature: Added support for manually setting that background image Feature: Added support for manually setting an app background image May 2, 2024
@yaira2 yaira2 force-pushed the ya/Image branch 3 times, most recently from e423068 to fc7eb6e Compare May 2, 2024 20:30
@yaira2 yaira2 marked this pull request as draft May 2, 2024 20:31
@yaira2 yaira2 changed the title Feature: Added support for manually setting an app background image Feature: Added a theming option to display a background image May 2, 2024
@yaira2 yaira2 force-pushed the ya/Image branch 2 times, most recently from 26c0c83 to c282477 Compare May 2, 2024 20:47
@yaira2 yaira2 marked this pull request as ready for review May 2, 2024 23:29
@yaira2 yaira2 requested review from 0x5bfa and hishitetsu May 2, 2024 23:29
0x5bfa
0x5bfa reviewed May 2, 2024
View reviewed changes
Copy link
Member

@0x5bfa 0x5bfa 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.

As of UI, why not put Browse button in the expander header and put Remove button as a normal button in the inner child showing path?

@yaira2 yaira2 mentioned this pull request May 3, 2024
Closed
0x5bfa
0x5bfa approved these changes May 3, 2024
View reviewed changes
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.

LGTM code wise

@yaira2 yaira2 merged commit 10bfa4b into main May 3, 2024
@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels May 3, 2024
@yaira2 yaira2 deleted the ya/Image branch May 3, 2024 02:01
hishitetsu
hishitetsu reviewed May 3, 2024
View reviewed changes
Copy link
Member

@hishitetsu hishitetsu left a comment

Choose a reason for hiding this comment

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

Sorry for the late review.

src/Files.App/ViewModels/Settings/AppearanceViewModel.cs
Comment on lines +69 to +81
// VerticalAlignment
ImageVerticalAlignmentTypes.Add(VerticalAlignment.Top, "Top".GetLocalizedResource());
ImageVerticalAlignmentTypes.Add(VerticalAlignment.Center, "Center".GetLocalizedResource());
ImageVerticalAlignmentTypes.Add(VerticalAlignment.Bottom, "Bottom".GetLocalizedResource());
ImageVerticalAlignmentTypes.Add(VerticalAlignment.Stretch, "Stretch".GetLocalizedResource());
SelectedImageVerticalAlignmentType = ImageVerticalAlignmentTypes[UserSettingsService.AppearanceSettingsService.AppThemeBackgroundImageVerticalAlignment];

// HorizontalAlignment
ImageHorizontalAlignmentTypes.Add(HorizontalAlignment.Left, "Top".GetLocalizedResource());
ImageHorizontalAlignmentTypes.Add(HorizontalAlignment.Center, "Center".GetLocalizedResource());
ImageHorizontalAlignmentTypes.Add(HorizontalAlignment.Right, "Bottom".GetLocalizedResource());
ImageHorizontalAlignmentTypes.Add(HorizontalAlignment.Stretch, "Stretch".GetLocalizedResource());
SelectedImageHorizontalAlignmentType = ImageHorizontalAlignmentTypes[UserSettingsService.AppearanceSettingsService.AppThemeBackgroundImageHorizontalAlignment];
Copy link
Member

Choose a reason for hiding this comment

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

  • Shouldn't horizontal alignment be left, center, or right?
  • Does Stretch make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just went with all the available options, but we can remove stretch if it doesn't make a difference.

Copy link
Member

Choose a reason for hiding this comment

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

Then we should remove stretch.

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

@hishitetsu hishitetsu hishitetsu left review comments

@0x5bfa 0x5bfa 0x5bfa approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: Add support for setting a background image for the app
3 participants
@yaira2 @0x5bfa @hishitetsu