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.

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

Feature: Add edit tags button to the preview pane #12946

Merged
merged 8 commits into from Jul 17, 2023

Conversation

ferrariofilippo
Copy link
Contributor

@ferrariofilippo ferrariofilippo commented Jul 13, 2023

Resolved / Related Issues

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 select a folder
    2. Open the preview pane
    3. Play with tags

Screenshots (optional)
Screenshot 2023-07-13 234208

@ferrariofilippo ferrariofilippo marked this pull request as draft July 13, 2023 21:50
@yaira2
Copy link
Member

yaira2 commented Jul 13, 2023

We may want to consider hiding this on items without tags, this will be consistent with other properties that are hidden when empty.

@mdtauk
Copy link

mdtauk commented Jul 14, 2023

I am assuming this is using the same flyout as is used elsewhere in the app - so there is no code duplication...

@ferrariofilippo
Copy link
Contributor Author

I am assuming this is using the same flyout as is used elsewhere in the app - so there is no code duplication...

Yes

@ferrariofilippo
Copy link
Contributor Author

We may want to consider hiding this on items without tags, this will be consistent with other properties that are hidden when empty.

I made it always visible since that was the required behaviour in the issue

Rather than hiding this when no tags have been attached to the item, we could put an "Add Tag" or "Add Tags" button.

Anyway, I think we should keep it even if there are no tags, it's quicker to edit tags using that button than using the context menu.
Still we could hide it if the user has turned off Show tags in context menu

@ferrariofilippo
Copy link
Contributor Author

image

@ferrariofilippo ferrariofilippo marked this pull request as ready for review July 14, 2023 12:00
@yaira2
Copy link
Member

yaira2 commented Jul 14, 2023

@ferrariofilippo Can you display the tags horizontally (and wrap onto the next line if there isn't enough room)?

@mdtauk
Copy link

mdtauk commented Jul 14, 2023

@ferrariofilippo Can you display the tags horizontally (and wrap onto the next line if there isn't enough room)?

Two per row I think will be enough, if they take up 50% of the space each, This way the tag icons below will align to the ones above, and it wont look staggered.

@yaira2
Copy link
Member

yaira2 commented Jul 14, 2023

Still we could hide it if the user has turned off

It's okay, we can always show it.

@ferrariofilippo
Copy link
Contributor Author

ferrariofilippo commented Jul 14, 2023

@yaira2 do you know any control better than ItemsWrapGrid? Using that you have to set manually items width
(I need that to display two tags/row using ItemsControl)

@yaira2
Copy link
Member

yaira2 commented Jul 16, 2023

@ferrariofilippo We can use a GridView with a custom template, the Community Toolkit has a couple of templates with the desired behavior.

@ferrariofilippo
Copy link
Contributor Author

@ferrariofilippo We can use a GridView with a custom template, the Community Toolkit has a couple of templates with the desired behavior.

Did you mean this?
https://learn.microsoft.com/en-us/windows/apps/design/controls/item-templates-gridview#icon-and-text

@ferrariofilippo
Copy link
Contributor Author

@yaira2
Copy link
Member

yaira2 commented Jul 16, 2023

Using that you can't display two tags/line, is that fine?

What do you mean? The screenshot looks like the desired behavior...

@ferrariofilippo
Copy link
Contributor Author

ferrariofilippo commented Jul 16, 2023

Two per row I think will be enough, if they take up 50% of the space each, This way the tag icons below will align to the ones above, and it wont look staggered.

I thought we were going for this
I'll push the commit then

yaira2
yaira2 previously approved these changes Jul 16, 2023
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

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 merged commit df9e87b into files-community:main Jul 17, 2023
2 checks passed
@yaira2
Copy link
Member

yaira2 commented Jul 17, 2023

image
I think we can improve the design by removing the "edit tag" button and instead, adding a "plus" icon to the GridView.

@ferrariofilippo ferrariofilippo deleted the Preview_Pane_Edit_Tags branch July 17, 2023 20:08
@ferrariofilippo
Copy link
Contributor Author

I think we can improve the design by removing the "edit tag" button and instead, adding a "plus" icon to the GridView.

Where should it be? Before/after the tags? Next to the header?

@yaira2
Copy link
Member

yaira2 commented Jul 17, 2023

Ideally it'll be in the list of tags, is this possible?

@ferrariofilippo
Copy link
Contributor Author

ferrariofilippo commented Jul 17, 2023

I'm not sure we can do that but I'll try

@ferrariofilippo
Copy link
Contributor Author

@yaira2 I think we can do that using something like this, is it fine?

public class TagsListItem
{
	public object Item { get; set; }

	public bool IsButton => object is Button;

	public bool IsTag => object is Tag;
}

<DataTemplate>
	<StackPanel>
		<TagLayout x:Load="{x:Bind obj.IsTag, Mode=OneWay}" other_props />
		<Button x:Load="{x:Bind obj.IsButton, Mode=OneWay}" />
	</StackPanel>
</DataTemplate>

@yaira2
Copy link
Member

yaira2 commented Jul 18, 2023

Seems like a bit of a hack but it's simple enough so we might as well.

@yaira2
Copy link
Member

yaira2 commented Jul 19, 2023

@ferrariofilippo even better, what if we added a "plus" icon next to the "Tags" header?

@ferrariofilippo
Copy link
Contributor Author

Does it look better using OpacityIcon or using the Glyph?

Screenshot 2023-07-19 102839
Screenshot 2023-07-19 103318

@Lukiluc29
Copy link
Contributor

Lukiluc29 commented Jul 19, 2023

Hello, sorry to embed myself in your discussion but it would not be better to put the button after the tags instead of putting it next to "tags"
254524240-81867ae9-fe5e-4dd7-ac09-e74a728721b2 (1)

@ferrariofilippo
Copy link
Contributor Author

Hello, sorry to embed myself in your discussion but it would not be better to put the button after the tags instead of putting it next to "tags" 254524240-81867ae9-fe5e-4dd7-ac09-e74a728721b2 (1)

This was the original idea but it's quite a mess to implement, we would need some hacky stuff

@Lukiluc29
Copy link
Contributor

Lukiluc29 commented Jul 19, 2023

It's a shame because the original idea/my idea is much more beautiful than the one you have just proposed, as, for me, it would look out of place next to the 'tags'.

@ferrariofilippo
Copy link
Contributor Author

@yaira2 what do you think? Both the solutions work properly. I can push the one you prefer

@yaira2
Copy link
Member

yaira2 commented Jul 19, 2023

@mdtauk what's your ideal design here?

@mdtauk
Copy link

mdtauk commented Jul 19, 2023

If it's going to be changed to a small button, it should use the Status Bar Button control we made, and should be right aligned by the Tags header. Buttons should typically be in a fixed location, adding it to the end of the tags list means it is always moving.

Is it going to be an edit button however, or an add tag button.

If it's editing, it should use the edit tag icon.

@yaira2 I did send you a design for the Tags themselves if you wanted to add the remove button to them.

If the button is an Add Tags button, then it should only add. Then you can have the remove icon appear on hover.

If it's an Edit button, then it would need to bring up a menu of tags, which allows toggling them on and off with an add button in the flyout.

image (1).png

@yaira2
Copy link
Member

yaira2 commented Jul 19, 2023

Is it going to be an edit button however, or an add tag button.

For now it's going to be the edit tag button.

@mdtauk
Copy link

mdtauk commented Jul 19, 2023

image
What about something like this. You can quick remove tags, or press the button to do more and add new ones.

@Lukiluc29
Copy link
Contributor

image What about something like this. You can quick remove tags, or press the button to do more and add new ones.

It's a great concept that will be beautiful once implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants