I9870 - Consolidate modal rendering stack #344
Conversation
| const options = ref(null); | ||
| const options2 = ref(null); | ||
| const modalStore = useModalStore(); | ||
| const {sideModal1, sideModal2} = storeToRefs(useModalStore()); |
There was a problem hiding this comment.
Perhaps the sideModal{1,2} can be renamed to something better (e.g. outer/innerModal)
There was a problem hiding this comment.
@jonasraoni There might be 3 levels in future.. so number actually works quite well.. And to be honest I hope that headlessui handles the stacking on their side - so I will be able to delete these work arounds.. (they have opened issue with this)
|
|
||
| // pkp.eventBus.$emit('open-tab', tab); | ||
| // Component can be either string or actual vue component | ||
| const component1 = computed(() => { |
There was a problem hiding this comment.
I'd create a function function createComputed(sideModal) { return computed(...); }; to decrease the duplicated code, but it's not important :)
There was a problem hiding this comment.
FYI I've just skimmed through the code, it's not possible to understand if everything makes sense without deep diving into the new code at the ui-library + the legacy one, but that would take a lot of time :)
It's a positive improvement. And as it's scheduled against +3.5, we'll have enough time to the flag problems.
|
@jonasraoni Thanks! Agreed.. I also think its impossible to guess all possible use cases that might have been there but maybe they already been replaced by Vue.js. So for this I rely quite a lot on cypress tests, which helped me to discover some of the more edge case situations (like delayed modal being closed while second one being opened). But it went actually quite well so hoping it solves more problems than it creates :-). |
No description provided.