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

Feature: Added support for opening all tagged items from the Tags widget #12972

Merged
merged 9 commits into from
Jul 19, 2023
Merged

Feature: Added support for opening all tagged items from the Tags widget #12972

merged 9 commits into from
Jul 19, 2023

Conversation

ferrariofilippo
Copy link
Contributor

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: Option to open all items in a tag #7493

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 app and navigate to Home
    2. Enable the Tags widget
    3. Right click on the title bar
    4. Click on Open all

Screenshots (optional)
Screenshot 2023-07-18 224854

@yaira2
Copy link
Member

yaira2 commented Jul 18, 2023 •
edited
Loading

@ferrariofilippo I pushed a design tweak so that clicking the tag name does the search for more items. This will allow us to repurpose the "view more" button as "open all".

@ferrariofilippo
Copy link
Contributor Author

What about using our OpacityIcons? The Glyph looks strange
image

QuaintMako
QuaintMako reviewed Jul 19, 2023
View reviewed changes
Copy link
Contributor

@QuaintMako QuaintMako left a comment

Choose a reason for hiding this comment

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

Code looks good, I am not able to test it.
A small proposition of renaming to stay coherent with the current.

src/Files.App/Actions/Content/Tags/OpenAllTaggedActions.cs Outdated
Comment on lines 12 to 13
public string Label
=> "OpenAllItems".GetLocalizedResource();
Copy link
Contributor

Choose a reason for hiding this comment

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

The label should be OpenAllTaggedItems

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

I thought this as well but then I realized this string might be useful outside of tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I revert it then?

Copy link
Member

Choose a reason for hiding this comment

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

It's fine either way

src/Files.App/Actions/Content/Tags/OpenAllTaggedActions.cs Outdated
Comment on lines 15 to 16
public string Description
=> "OpenAllItemsDescription".GetLocalizedResource();
Copy link
Contributor

Choose a reason for hiding this comment

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

OpenAllTaggedItemsDescription

src/Files.App/Strings/en-US/Resources.resw Outdated
Comment on lines 3401 to 3406
<data name="OpenAllItems" xml:space="preserve">
<value>Open all</value>
</data>
<data name="OpenAllItemsDescription" xml:space="preserve">
<value>Open all tagged items</value>
</data>
Copy link
Contributor

Choose a reason for hiding this comment

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

OpenAllTaggedItems and OpenAllTaggedItemsDescription

src/Files.App/UserControls/Widgets/FileTagsWidget.xaml Outdated
Comment on lines 88 to 90
AutomationProperties.Name="{helpers:ResourceString Name=OpenAllItems}"
Command="{x:Bind OpenAllCommand}"
ToolTipService.ToolTip="{helpers:ResourceString Name=OpenAllItems}">
Copy link
Contributor

Choose a reason for hiding this comment

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

If the resources strings are indeed renamed as proposed, these entries will need to be renamed as well.

@yaira2
Copy link
Member

yaira2 commented Jul 19, 2023 •
edited
Loading

What about using our OpacityIcons? The Glyph looks strange image

@ferrariofilippo I don't think the opacity icon is a good fit but to be honest, there isn't an intuitive icon for "open all". An alternative would be to use the more icon and display the menu item to "open all" when clicking the icon.
image

@ferrariofilippo
Copy link
Contributor Author

@ferrariofilippo I don't think the opacity icon is a good fit but to be honest, there isn't an intuitive icon for "open all". An alternative would be to use the more icon and display the menu item to "open all" when clicking the icon.

Good call, we won't need to change the code if we will need any extra actions

@ferrariofilippo
Copy link
Contributor Author

image
Should we remove the button border?

@yaira2
Copy link
Member

yaira2 commented Jul 19, 2023

Can you set it to transparent?

@ferrariofilippo
Copy link
Contributor Author

image

@yaira2
Copy link
Member

yaira2 commented Jul 19, 2023

Does this also work for the sidebar?

@ferrariofilippo
Copy link
Contributor Author

Not yet, I'll work on it

yaira2
yaira2 approved these changes Jul 19, 2023
View reviewed changes
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 Jul 19, 2023
@yaira2 yaira2 merged commit b26e831 into files-community:main Jul 19, 2023
@yaira2 yaira2 changed the title Feature: Open all items in a tag Feature: Added support for opening all tagged items from the Tags widget Jul 19, 2023
@ferrariofilippo ferrariofilippo deleted the Open_All_Items_Tag branch July 19, 2023 15:16
@ferrariofilippo ferrariofilippo mentioned this pull request Jul 20, 2023
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuaintMako QuaintMako QuaintMako left review comments

@yaira2 yaira2 yaira2 approved these changes

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: Option to open all items in a tag
3 participants
@ferrariofilippo @yaira2 @QuaintMako