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

feat(VNavigationDrawer): port to v3 #13115

Merged
merged 27 commits into from
Mar 15, 2021
Merged

feat(VNavigationDrawer): port to v3 #13115

merged 27 commits into from
Mar 15, 2021

Conversation

johnleider
Copy link
Member

Description

port to v3

Motivation and Context

port to v3

How Has This Been Tested?

pending

Markup:

https://gist.github.com/johnleider/2961f4bcd3831fc37f8ea2ff4061137e

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement/refactoring (non-breaking change that doesn't add any features but makes things better)

Checklist:

  • The PR title is no longer than 64 characters.
  • The PR is submitted to the correct branch (master for bug fixes and documentation updates, dev for new features and backwards compatible changes and next for non-backwards compatible changes).
  • My code follows the code style of this project.
  • I've added relevant changes to the documentation (applies to new features and breaking changes in core library)

@johnleider johnleider force-pushed the feat/v3-navigation-drawer branch 2 times, most recently from b2e2d22 to 52e609f Compare February 23, 2021 17:36
@johnleider johnleider force-pushed the feat/v3-navigation-drawer branch 2 times, most recently from f1217e7 to 8b9bcf1 Compare March 2, 2021 21:50
@johnleider johnleider added this to the v3.0.0 milestone Mar 3, 2021
@johnleider johnleider added this to In progress in Vuetify 3 - Titan via automation Mar 3, 2021
@johnleider johnleider self-assigned this Mar 3, 2021
@johnleider johnleider marked this pull request as ready for review March 11, 2021 23:29
@johnleider
Copy link
Member Author

Requesting Initial comments from @vuetifyjs/core-team

@johnleider johnleider requested a review from a team March 11, 2021 23:29
@johnleider johnleider added the C: VNavigationDrawer VNavigationDrawer label Mar 11, 2021
@nekosaur
Copy link
Member

nekosaur commented Mar 12, 2021

I think it would be very helpful if you provided descriptions of some of the props available here. Some of my immmediate questions are:

  1. What's the difference between mobile and temporary? They seem to have the same effect, even on small screens.
  2. temporary and floating are too close in meaning imo. It would not be hard to imagine that someone sees floating and thinks that it does what temporary actually does.
  3. What does permanent do?
  4. What does stateless do? It sounds a little bit like temporary as well.
  5. bottom makes nav drawer take up entire screen
  6. On the topic of bottom (and right), I am not a fan of using the position composable but changing the meaning of the props. In nav drawer, bottom and right are used only as boolean props to indicate where drawer should show (while the logic and calculation of that is done by layout composable), but they are defined as [Boolean, Number, String]. I would much rather see a position prop that takes 'left' | 'right' | 'bottom'.
  7. Because it uses position composable it also has top prop which doesn't make sense.
  8. makeLayoutItemProps should not set default name. That would immediately trip up the layout system when two nav drawers are used.
  9. What's the use case for fixed? Right now it gets overwritten by layout which sets absolute position in styles

@nekosaur
Copy link
Member

Elaboration on some of my questions:

  1. Going by name only, floating could be interpreted to do what temporary does, while it is not at all obvious from the name that it only removes border.
  2. In v2, permanent is described as The drawer remains visible regardless of screen size. Would this not be equal to
    :model-value="true"
    
    since we're using the proxy composable. If mobile/route changing is still wanted, then it would be
    v-model="foo"
    mobile-breakpoint="0"
    
  3. In v2, stateless is described as Remove all automated state functionality (resize, mobile, route) and manually control the drawer state. Would this not be
    :model-value="foo"
    

@johnleider
Copy link
Member Author

  1. mobile is a temporary prop to test the mobile functionality of a component and will go away once the display composable is done.
  2. Agreed, only done for v2 feature parity. floating can be removed for border="0"
  3. We can remove stateless, but permanent should stay.
    a. That will work as well
  4. It can go
  5. Should only happen on landscape, I can remove for now and implement it once display is in
  6. I'm not opposed to this
  7. The same as 6
  8. Okay
  9. So that being fixed isn't tied to the usage of app, at least in v2

Vuetify 3 - Titan automation moved this from In progress to Review in progress Mar 15, 2021
Vuetify 3 - Titan automation moved this from Review in progress to Reviewer approved Mar 15, 2021
@johnleider johnleider merged commit 95139d7 into next Mar 15, 2021
Vuetify 3 - Titan automation moved this from Reviewer approved to Done Mar 15, 2021
@johnleider johnleider deleted the feat/v3-navigation-drawer branch March 15, 2021 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: VNavigationDrawer VNavigationDrawer
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants