-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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(VExpansionPanels): port to v3 #14014
Conversation
fad6936
to
554c6ab
Compare
554c6ab
to
a25d10a
Compare
packages/vuetify/src/components/VExpansionPanel/VExpansionPanel.sass
Outdated
Show resolved
Hide resolved
packages/vuetify/src/components/VExpansionPanel/VExpansionPanel.tsx
Outdated
Show resolved
Hide resolved
eager: Boolean, | ||
}, 'lazy') | ||
|
||
export function useLazy (props: { eager: boolean }, active: Ref<boolean>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably replace useBooted
in VOverlay with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And delete bootable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed bootable but have not touched VOverlay. I wasn't sure what you meant by comment below, if the current onAfterLeave
is broken or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I'd broken it because there isn't anything in useBooted that sets it back to false, then I realised it's actually in an external event handler. Ideally all the lazy components should have the same behaviour, so onAfterLeave
should maybe be part of the composable instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha
|
||
export function useLazy (props: { eager: boolean }, active: Ref<boolean>) { | ||
const isBooted = ref(false) | ||
const hasContent = computed(() => isBooted.value || props.eager || active.value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also #6764 which I think I broke again at some point
vuetify/packages/vuetify/src/components/VOverlay/VOverlay.tsx
Lines 191 to 193 in fedb40b
function onAfterLeave () { | |
if (!props.eager) isBooted.value = false | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few points:
- Is there a way that we can avoid having to use !important?
- I love the re-use of the variant nomenclature
- What's your stance on density support?
packages/vuetify/src/components/VExpansionPanel/VExpansionPanel.sass
Outdated
Show resolved
Hide resolved
packages/vuetify/src/components/VExpansionPanel/VExpansionPanel.sass
Outdated
Show resolved
Hide resolved
packages/vuetify/src/components/VExpansionPanel/VExpansionPanel.sass
Outdated
Show resolved
Hide resolved
packages/vuetify/src/components/VExpansionPanel/VExpansionPanel.sass
Outdated
Show resolved
Hide resolved
packages/vuetify/src/components/VExpansionPanel/VExpansionPanelContent.tsx
Outdated
Show resolved
Hide resolved
packages/vuetify/src/components/VExpansionPanel/VExpansionPanelContent.tsx
Outdated
Show resolved
Hide resolved
packages/vuetify/src/components/VExpansionPanel/VExpansionPanels.tsx
Outdated
Show resolved
Hide resolved
packages/vuetify/src/components/VExpansionPanel/_variables.scss
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
collapse-icon
prop to match other components with two icon states
I wonder if something like this would be a good idea
{ expandIcon && collapseIcon
? <VRotateTransition>...icons somehow
: just the one icon
Is there a way that we can avoid having to use !important?
Would be nice to get rid of all those :not()
s too
I agree. However this is the solution I managed to come up with. The |
It's probably something that should be explored outside of this PR, since it would apply to several places/components |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go for it, if it needs more work we can clean up later.
Description
#13482
closes #13551
BREAKING CHANGES
v-expansion-panels
accordion
,inset
andpopup
props removed, replaced with combinedvariant
propdark
andlight
props replaced bytheme
propfocusable
prop removed. component is focusable by defaulthover
prop removed. component has hover state by defaulttile
prop removedflat
prop removedv-expansion-panel
rounded
andtile
props through rounded composableelevation
prop through elevation composabletag
prop through tag composablecontent
andheader
props and slotscolor
(header) andbg-color
(content)readonly
(this is just :model-value"")v-expansion-panel-header
v-expansion-panel-title
disable-icon-rotate
prop. icon does not rotate by defaultcollapse-icon
prop to match other components with two icon states (see v-checkbox)v-expansion-panel-text
TODO
Motivation and Context
v3
How Has This Been Tested?
playground, cypress tests
Markup:
Types of changes
Checklist:
master
for bug fixes and documentation updates,dev
for new features and backwards compatible changes andnext
for non-backwards compatible changes).