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

Lookless file dialogs #4615

Merged
merged 11 commits into from
May 1, 2021
Merged

Lookless file dialogs #4615

merged 11 commits into from
May 1, 2021

Conversation

Splitwirez
Copy link
Contributor

@Splitwirez Splitwirez commented Sep 3, 2020

What does the pull request do?

Makes the Managed File Dialogs lookless, so they can be retemplated.

With Default theme:
Screenshot_20200903_193214

With Fluent theme:
Screenshot_20210423_152042
(formerly Screenshot_20200903_193005 )

What is the current behavior?

Managed File Dialogs are UserControls, and thus cannot be readily customized

What is the updated/expected behavior with this PR?

Increased cosmetic flexibility

How was the solution implemented (if it's not obvious)?

It's kind of obvious, no?

Checklist

Breaking changes

I suppose merging this would mean the dialogs would no longer "just work" in an Avalonia app which does not merge either of Avalonia's built-in themes - such an app would now have to provide its own templates.

@danwalmsley
Copy link
Member

@Splitwirez im going to hold off merging this until after 0.10.0 release... so quite soon after that... 0.10.1 release.

Just because this control is so critical to many applications... we cant afford to break them or introduce a tiny subtle bug...

however I like the PR.. its very good and we will merge it after the release.

@Splitwirez
Copy link
Contributor Author

@Splitwirez im going to hold off merging this until after 0.10.0 release... so quite soon after that... 0.10.1 release.

Just because this control is so critical to many applications... we cant afford to break them or introduce a tiny subtle bug...

however I like the PR.. its very good and we will merge it after the release.

That makes perfect sense to me. I'll try to get the XAML cleaned up in the meantime.

@Splitwirez
Copy link
Contributor Author

@danwalmsley So uh...now that 0.10.0 is out...has the time come for this PR?

@maxkatz6
Copy link
Member

@danwalmsley @grokys do we want to merge it into 0.10.1 or wait for starting work on next big release (0.11 or 11.0)?

@Splitwirez
Copy link
Contributor Author

@maxkatz6 @danwalmsley It just hit me earlier today that there actually is a potential breaking change to come of this PR, albeit a rather...obscure one. I've just updated the initial PR post to explain what that breaking change would be. Mentioning both of you, to make sure this PR isn't merged without the breaking change being taken into account.

I am so sorry I didn't figure this out sooner...

@maxkatz6
Copy link
Member

@Splitwirez I believe this breaking change is pretty expected. Still good to mention it, thanks.
Meanwhile, can you fix conflicts?

@Splitwirez
Copy link
Contributor Author

@Splitwirez I believe this breaking change is pretty expected. Still good to mention it, thanks.
Meanwhile, can you fix conflicts?

@maxkatz6 Glad to hear I'm the only one who didn't see that breaking change at first.

I'm working on fixing the conflicts as we speak. After that, I'm going to make sure the Fluent look and icons are in line with what Microsoft has unveiled recently. Once those are both taken care of, this should be ready to be merged (unless you folks want other changes made ofc)

@Splitwirez
Copy link
Contributor Author

...update: I am currently banging my head against the wall trying to figure out some technical difficulties regarding compiling Avalonia while also coming face to face with how git-illiterate I am. Hopefully this won't take too much longer...?

@Splitwirez
Copy link
Contributor Author

@maxkatz6 Okay, the conflicts are resolved, icons updated, and the Fluent theme styles actually look...well, Fluent now. I say it's ready to be merged...unless you folks want further changes made, of course.

@@ -3543,7 +3543,8 @@
"ansi-regex": {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this should be changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't touch that...in fact, I don't even know what it is...

Copy link
Member

Choose a reason for hiding this comment

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

NPM packages lock file, it's safe to just revert changes in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...how exactly do I do that here?

Copy link
Member

Choose a reason for hiding this comment

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

Easy way - just copy file without changes from the repo and replace in your local directory, commit.
Harder way - rebase, discard changes in this file on the specific commit, where it was changed.

Either way is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, done and done 👍

Copy link
Member

@maxkatz6 maxkatz6 left a comment

Choose a reason for hiding this comment

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

Thanks!

@maxkatz6 maxkatz6 enabled auto-merge May 1, 2021 07:22
@maxkatz6 maxkatz6 merged commit 1094509 into AvaloniaUI:master May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants