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(VDivider): port to v3 #12993

Merged
merged 14 commits into from
Jan 26, 2021
Merged

feat(VDivider): port to v3 #12993

merged 14 commits into from
Jan 26, 2021

Conversation

johnleider
Copy link
Member

@johnleider johnleider commented Jan 20, 2021

Description

More to follow..

fixes #12054

Motivation and Context

Port v-divider to v3

How Has This Been Tested?

jest

Markup:

https://gist.github.com/johnleider/9b501dc94fa13e75ed339f425613f115

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 changed the base branch from master to next January 20, 2021 18:48
@johnleider johnleider self-assigned this Jan 20, 2021
@johnleider johnleider added the C: VDivider VDivider label Jan 20, 2021
@johnleider johnleider added this to In progress in Vuetify 3 - Titan via automation Jan 20, 2021
@johnleider johnleider added this to the v3.0.0 milestone Jan 20, 2021
@johnleider
Copy link
Member Author

Requesting Initial comments from @vuetifyjs/contributors @vuetifyjs/core-team

@johnleider johnleider changed the title Feat/v divider feat(VDivider): port component to v3 Jan 20, 2021
@vuetifyjs vuetifyjs deleted a comment from nekosaur Jan 20, 2021
@johnleider johnleider changed the title feat(VDivider): port component to v3 feat(VDivider): port to v3 Jan 21, 2021
@johnleider johnleider force-pushed the feat/v-divider branch 2 times, most recently from c8b6ea6 to 19a6d5f Compare January 22, 2021 22:20
@johnleider johnleider marked this pull request as ready for review January 22, 2021 22:25
@johnleider
Copy link
Member Author

Requesting Final comments from @vuetifyjs/contributors @vuetifyjs/core-team

Copy link
Member

@nekosaur nekosaur left a comment

Choose a reason for hiding this comment

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

I'm not entirely on board with the current API of v-divider. Assuming everything works correctly, the following markup

<v-divider width="100px" height="100px" border></v-didiver>

Would result in a 100x100px box with border on all sides. That seems very far from what v-divider should be and do.

Additionally, markup like this

<v-divider width="100px"></v-divider>

Would result in something like this in horizontal mode

image

And this in vertical mode

image

since width/height are orientation dependent.

In my mind, the props

border
width
height
maxWidth
maxHeight

could be replaced by

thickness
length
color (if we want to keep that functionality from border)

and better represent the function of the component

Vuetify 3 - Titan automation moved this from In progress to Reviewer approved Jan 26, 2021
Co-authored-by: Albert Kaaman <albert@kaaman.nu>
@johnleider johnleider merged commit cc033c6 into next Jan 26, 2021
Vuetify 3 - Titan automation moved this from Reviewer approved to Done Jan 26, 2021
@johnleider johnleider deleted the feat/v-divider branch January 26, 2021 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: VDivider VDivider T: feature A new feature
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants