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: Disable warning when a new name is empty #12150

Merged
merged 20 commits into from
Apr 24, 2023
Merged

Feature: Disable warning when a new name is empty #12150

merged 20 commits into from
Apr 24, 2023

Conversation

ferrariofilippo
Copy link
Contributor

@ferrariofilippo ferrariofilippo commented Apr 21, 2023 •
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: Disable warning when a new name is empty #12140

Removed from

  • New Item Dialog
  • New Archive Dialog
  • New Shortcut Dialog

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
    2. Open the menu to create a new file
    3. See that the warning is not displayed when the name is empty

@yaira2
Copy link
Member

yaira2 commented Apr 21, 2023

Should we target the servicing branch?

@ferrariofilippo
Copy link
Contributor Author

ferrariofilippo commented Apr 21, 2023 •
edited
Loading

I don't think this issue is that urgent. I mean, it's not fixing a breaking bug

yaira2
yaira2 previously approved these changes Apr 21, 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 ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Apr 21, 2023
@ferrariofilippo
Copy link
Contributor Author

LGTM

What do you think about the two questions in the description?

@yaira2
Copy link
Member

yaira2 commented Apr 21, 2023

Should we disable the tip also when the shortcut path is invalid?
I believe we shouldn't remove that from tags. What do you think?

Is there any reason to handle it differently?

@ferrariofilippo
Copy link
Contributor Author

Is there any reason to handle it differently?

For the first one (shortcuts) I agree, we should not show the tip. It was not mentioned in the issue and I wanted to wait on some feedback to avoid wasting work
For tags, I don't think the warning is disturbing there, I mean it's not covering anything else (as the tip does)

@yaira2
Copy link
Member

yaira2 commented Apr 21, 2023

We can try without it then 👍

@hishitetsu
Copy link
Member

hishitetsu commented Apr 21, 2023 •
edited
Loading

I think this should be merged into the servicing brunch.
It is certainly not a breaking bug, but I think it is making the UX much worse.

@hishitetsu
Copy link
Member

And I think the tag warning should be removed. The problem is not that something is hidden, but that the warnings are just unnecessary and offensive to the user.

@ferrariofilippo
Copy link
Contributor Author

And I think the tag warning should be removed. The problem is not that something is hidden, but that the warnings are just unnecessary and offensive to the user.

I removed it for empty names but not for whitespaces

@ferrariofilippo ferrariofilippo changed the base branch from main to service/2.4.67 April 22, 2023 06:39
@ferrariofilippo
Copy link
Contributor Author

I think this should be merged into the servicing brunch. It is certainly not a breaking bug, but I think it is making the UX much worse.

I swapped it to servicing but the changes are like 3k lines (instead of something like 20). I think I should change back to main

@ferrariofilippo ferrariofilippo changed the base branch from service/2.4.67 to main April 22, 2023 07:09
@hishitetsu
Copy link
Member

hishitetsu commented Apr 22, 2023 •
edited
Loading

I swapped it to servicing but the changes are like 3k lines (instead of something like 20). I think I should change back to main

Perhaps you have to create another branch based on the servicing branch and cherry-pick your commits into it.
Or maybe rebase would work.
git rebase --onto service/2.4.67 main Remove_Invalid_Warning_On_Empty

0x5bfa and others added 8 commits April 22, 2023 09:34
@0x5bfa @ferrariofilippo
Feature: Integrate Advanced Security page into properties window (fil…
d621dff 
…es-community#11930)
@0x5bfa @ferrariofilippo
Code Quality: Introduce Implicit Usings (files-community#12121)
3425fb9
@dependabot @ferrariofilippo
Build(deps): Bump Microsoft.DotNet.UpgradeAssistant.Extensions.Defaul…
c15eb22 
…t.Analyzers from 0.4.410601 to 0.4.421302 (files-community#12134)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@0x5bfa @ferrariofilippo
Code Quality: Added license header to C# phase1 (files-community#12143)
5d119d8
@ferrariofilippo
Feature: Disable warning when a new name is empty
16ea03c
@ferrariofilippo
Remove warning for empty shortcut
c9afcd8
@ferrariofilippo
Remove tag warning
70c04d1
@ferrariofilippo
Merge branch 'Remove_Invalid_Warning_On_Empty' of https://github.com/…
0ab07d0 
…ferrariofilippo/Files into Remove_Invalid_Warning_On_Empty
@yaira2 yaira2 merged commit 7ed84ac into files-community:main Apr 24, 2023
hishitetsu pushed a commit to hishitetsu/Files that referenced this pull request Apr 24, 2023
@ferrariofilippo @hishitetsu
Feature: Disable warning when a new name is empty (files-community#12150
5db16e4 
)
@ferrariofilippo ferrariofilippo deleted the Remove_Invalid_Warning_On_Empty branch April 25, 2023 05:31
@ferrariofilippo ferrariofilippo mentioned this pull request Apr 28, 2023
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaira2 yaira2 yaira2 left review comments

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: Disable warning when a new name is empty
4 participants
@ferrariofilippo @yaira2 @hishitetsu @0x5bfa