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 invalid tag name warning #11324

Merged
merged 19 commits into from
Mar 22, 2023
Merged

Conversation

heftymouse
Copy link
Contributor

@heftymouse heftymouse commented Feb 16, 2023

Resolved / Related Issues
Items resolved / related issues by this PR.

Validation
How did you test these changes?

  • Built and ran the app
  • Tested the changes for accessibility

Screenshots (optional)
Add screenshots here.

@heftymouse
Copy link
Contributor Author

Narrator doesn't read the text but I'm not sure how to solve it

@heftymouse heftymouse marked this pull request as draft February 16, 2023 12:56
@yaira2
Copy link
Member

yaira2 commented Feb 16, 2023

Narrator doesn't read the text but I'm not sure how to solve it

There is a setting to read it but I think it's okay as long as the text property is it.

@yaira2
Copy link
Member

yaira2 commented Feb 16, 2023

@heftymouse the tag settings were moved to a separate page

@heftymouse heftymouse reopened this Feb 16, 2023
@heftymouse heftymouse marked this pull request as ready for review February 16, 2023 16:08
@yaira2 yaira2 added the changes requested Changes are needed for this pull request label Feb 17, 2023
@yaira2 yaira2 added needs - code review and removed changes requested Changes are needed for this pull request labels Feb 17, 2023
@heftymouse
Copy link
Contributor Author

heftymouse commented Feb 17, 2023 via email

Copy link
Contributor

@ferrariofilippo ferrariofilippo left a comment

Choose a reason for hiding this comment

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

Can you solve the UX issue?

src/Files.App/Views/SettingsPages/Tags.xaml.cs Outdated Show resolved Hide resolved
@QuaintMako
Copy link
Contributor

It would be nice to have a tooltip when hovering on the "Invalid Name" telling what are the rules of naming a tag? Plus, having just the shortcut to cancel the edit in the tooltip is a bit weird.

image

@heftymouse
Copy link
Contributor Author

Good idea, I'll do that
However the Esc tooltip is there for all the elements in the editing UI because of the keyboard accelerator lol

@yaira2
Copy link
Member

yaira2 commented Mar 12, 2023

This has been fixed, I'll fix the conflicts when it needs to be merged

Which resource was it?

@heftymouse
Copy link
Contributor Author

heftymouse commented Mar 13, 2023

Which resource was it?

I had to retemplate the control to centre align its content

@yaira2
Copy link
Member

yaira2 commented Mar 15, 2023

I had to retemplate the control to centre align its content

I was hoping we could do it with a single resource, if we have to restyle the whole control, it makes me wonder if the original implementation with a grid was cleaner. What are your thoughts on this?

@heftymouse
Copy link
Contributor Author

I agree

@yaira2
Copy link
Member

yaira2 commented Mar 16, 2023

Can you fix the merge conflicts?

@yaira2
Copy link
Member

yaira2 commented Mar 19, 2023

Can you share a screenshot?

@heftymouse
Copy link
Contributor Author

image

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 previously approved these changes Mar 19, 2023
hecksmosis
hecksmosis previously approved these changes Mar 21, 2023
@yaira2
Copy link
Member

yaira2 commented Mar 21, 2023

@heftymouse can you fix the merge conflict?

@heftymouse heftymouse dismissed stale reviews from hecksmosis and yaira2 via feb3663 March 22, 2023 06:31
@yaira2
Copy link
Member

yaira2 commented Mar 22, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@ferrariofilippo ferrariofilippo left a comment

Choose a reason for hiding this comment

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

There's an issue where you can rename a tag using the name of an existing tag (but you cannot create it)

4

Just found another issue:
if you clear the tag name and hit Enter the name will be set to an empty string (still the warning is displayed correctly)

src/Files.App/Views/Settings/TagsPage.xaml Outdated Show resolved Hide resolved
src/Files.App/Views/Settings/TagsPage.xaml Outdated Show resolved Hide resolved
src/Files.App/Views/Settings/TagsPage.xaml Outdated Show resolved Hide resolved
src/Files.App/Views/Settings/TagsPage.xaml Outdated Show resolved Hide resolved
src/Files.App/Views/Settings/TagsPage.xaml.cs Outdated Show resolved Hide resolved
src/Files.App/Views/Settings/TagsPage.xaml.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@ferrariofilippo ferrariofilippo 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 ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Mar 22, 2023
@yaira2 yaira2 merged commit 4058cc7 into files-community:main Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Warn the user about invalid tag name
5 participants