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

fix(VDivider): visiblity in container with dynamic height #19828

Merged
merged 1 commit into from
May 21, 2024

Conversation

J-Sek
Copy link
Contributor

@J-Sek J-Sek commented May 16, 2024

Description

Assuming <v-divider vertical> makes sense only inside flexbox container, the height: auto allows align-self: stretch to take effect when container does not have explicit height.

Example in docs does not expose the issue, because d-flex gets additional explicit height, but in real life, we usually leave height to be dynamic and determined based on the children height.

After the change I did not notice any negative effects, but I may not know about use cases outside flexbox.

fixes #19827

@MajesticPotatoe MajesticPotatoe added T: bug Functionality that does not work as intended/expected C: VDivider VDivider labels May 17, 2024
@KaelWD KaelWD changed the base branch from dev to master May 21, 2024 12:05
@KaelWD KaelWD added this to the v3.6.x milestone May 21, 2024
@KaelWD KaelWD merged commit ea567bb into vuetifyjs:master May 21, 2024
8 checks passed
@J-Sek
Copy link
Contributor Author

J-Sek commented May 23, 2024

@johnleider I can see that translating length prop into max-height or max-width (depending on orientation) is a 3 year old decision, but if we could change it to height and width, the example use case would work. Are there any arguments against this idea?

edit 1: Quick workaround would be h-100 class next to align-self-center. Although it would be quite unintuitive.

edit 2: There was this guy in 2020 (#12054) with use case similar to this. Nowdays, his code does not require any additional styles (if we ignore ml-9 to center the line for mobile viewport). Do we know which is more popular? inset or length + .align-self-center?

@johnleider
Copy link
Member

I feel having to use the utility classes is unintuitive. Whatever changes are made, we should probably add some e2e tests to make sure this doesn't happen again. For now, I'm not sure the best path, however, I'm leaning towards changing to width/height from max width/height.

@J-Sek
Copy link
Contributor Author

J-Sek commented May 24, 2024

I have spent a bit of time reviewing paid themes based on Vuetify and found 2 cases where going back to height: 100% would make the divider disappear [1], [2]. So the most sensible option seems to be an update to length prop. The PR includes couple of test cases, let me know if they are an overkill for such a small component. They run pretty fast on my machine, but I don't want to impact CI/CD processing times more than it is necessary.

edit: Two more test cases will detect changing back to inherit. It appears that height: inherit was not breaking any of the scenarios that were originally implemented. The commit 72e5319 replacing inherit is not linked to any scenario, so I am open to suggestions if you'd feel like we need more.

@johnleider
Copy link
Member

Want to take this under your wing @J-Sek ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: VDivider VDivider T: bug Functionality that does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug Report][3.6.6] VDivider vertical not visible due to height 100% resulting in 0px
4 participants