Skip to content

Conversation

soumyamahunt
Copy link
Contributor

This pr tries to implement feature requested in #672.

@soumyamahunt
Copy link
Contributor Author

Currently sidebar can only be resized up to certain width.

@yaira2
Copy link
Member

yaira2 commented Jul 19, 2020

Currently sidebar can only be resized up to certain width.

@soumyamahunt That would be the expected behavior. Does the app keep track of the size across sessions?

@soumyamahunt
Copy link
Contributor Author

Currently sidebar can only be resized up to certain width.

@soumyamahunt That would be the expected behavior. Does the app keep track of the size across sessions?

No not now, I will add it.

@lukeblevins
Copy link
Contributor

@soumyamahunt If you need any inspiration, check out the appropriate branch for my past, unfinished work on this. Looks really good so far.

@soumyamahunt
Copy link
Contributor Author

@yaichenbaum @duke7553 what should we keep minimum and maximum sidebar width??

@yaira2
Copy link
Member

yaira2 commented Jul 19, 2020

@yaichenbaum @duke7553 what should we keep minimum and maximum sidebar width??

I would recommend keeping the current size as the minimum.

@ghost ghost added the needs - code review label Jul 19, 2020
@jaigak
Copy link
Contributor

jaigak commented Jul 20, 2020

@yaichenbaum To match Fluent Design Guidelines, the minimum width must be at least 20% of the container.

@jaigak
Copy link
Contributor

jaigak commented Jul 20, 2020

@soumyamahunt Could you share a screenshot on the changes?

@soumyamahunt
Copy link
Contributor Author

@soumyamahunt Could you share a screenshot on the changes?

Here is how it behaves with a minimum and maximum value.
R7Mjnsy4Dx

@soumyamahunt
Copy link
Contributor Author

@yaichenbaum, @duke7553 minimum width is kept at 200(as was before) maximum is kept at 500. Also, width is remembered across instances.

R7Mjnsy4Dx

@jaigak
Copy link
Contributor

jaigak commented Jul 20, 2020 via email

@soumyamahunt
Copy link
Contributor Author

Can you increase the minimum width to match the default WinUI styles?

I can but @yaichenbaum suggested to keep it same as before, @yaichenbaum what do you think of this?? Also what is the default width value in WinUI style??

@lukeblevins
Copy link
Contributor

@soumyamahunt Nope, we're happy with the existing minimum.

@yaira2 yaira2 self-requested a review July 20, 2020 19:17
@yaira2 yaira2 added changes requested Changes are needed for this pull request and removed needs - code review labels Jul 21, 2020
@jaigak
Copy link
Contributor

jaigak commented Jul 21, 2020

@soumyamahunt Nope, we're happy with the existing minimum.

Now the settings sidebar looks out of place. Could you use the same size for both NavigationView controls and also make Setting sidebar resizable?

@soumyamahunt
Copy link
Contributor Author

Now the settings sidebar looks out of place. Could you use the same size for both NavigationView controls and also make Setting sidebar resizable?

I don't think making settings sidebar resizable gonna improve any usability since the strings used there are static, but I can make it sync with the main sidebar. @yaichenbaum, is this something you would like too??

@yaira2
Copy link
Member

yaira2 commented Jul 21, 2020

The settings sidebar should not be resizable.

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed changes requested Changes are needed for this pull request labels Jul 22, 2020
@yaira2 yaira2 changed the title Allow resizing sidebar Added the ability to resize the sidebar Jul 22, 2020
@yaira2 yaira2 merged commit e6d057a into files-community:master Jul 22, 2020
@soumyamahunt soumyamahunt deleted the resize-sidebar branch July 22, 2020 02:28
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.

4 participants