feat: Lean more on vue slots for state#148
Conversation
richipargo
changed the title
|
Still have to document the changes |
|
@richipargo Hey Ricardo! This is big one! Thanks for contributing it! Could you please elaborate a little bit what problem does this PR solve? |
|
Hey @paveltiunov, more than a problem the intention of this PR is to give more flexibility on the usage of the Still i'm missing a couple of items so I would leave it open still but comments and suggestions are welcome |
richipargo
changed the title
|
@paveltiunov It's ready! |
|
@richipargo Hey Ricardo! Cool! Are these changes backward compatible with previous version? |
|
Mostly yes
…On Tue, Jul 9, 2019, 21:37 Pavel Tiunov ***@***.***> wrote:
@richipargo <https://github.com/richipargo> Hey Ricardo! Cool! Are these
changes backward compatible with previous version?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#148>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABHC6HYC3F4IQIWEQHWHVBDP6TSIFANCNFSM4H4SMBDQ>
.
|
| 'div', | ||
| { | ||
| class: { | ||
| 'cubejs-query-renderer': true, |
There was a problem hiding this comment.
Do you use this one for styling?
|
The idea is to make the object selectable if it requires styling, but of
course I do use it
…On Tue, Jul 9, 2019, 22:03 Pavel Tiunov ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/cubejs-vue/src/QueryRenderer.js
<#148 (comment)>:
> }
+
+ return createElement(
+ 'div',
+ {
+ class: {
+ 'cubejs-query-renderer': true,
Do you use this one for styling?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#148>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABHC6H4XJEHMPBNMYMVH6F3P6TVJFANCNFSM4H4SMBDQ>
.
|
|
@richipargo Is there any chance we can make it fully backward compatible? I know some guys already deployed it. It would be great if we can make every version upgrade as smooth as possible. |
|
I wanted to avoid it cause it generated a lot of noise, but i've set back into the default slot the missing variables to mantain compatibility |
| ); | ||
| slot = $scopedSlots.default ? $scopedSlots.default(slotProps) : slot; | ||
| } else if (error) { | ||
| slot = $scopedSlots.error ? $scopedSlots.error({ error, sqlQuery }) : slot; |
There was a problem hiding this comment.
Do you think we should fallback here to default if only one slot is passed for backward compatibility?
There was a problem hiding this comment.
You're right i missed that one
|
@richipargo Hey Ricardo! Everything looks great! The only thing: is there any chance it can work without class names? Would be really great if we can maintain code to be class name free in order to prevent any class name clash. |
|
@paveltiunov while i would like to keep it I agree with the possible clashing, so I removed it |
|
@richipargo Cool! Thanks! |
* Add support for empty/error/valid states on QueryBuilder * query testing * adjustements on member changing and updating * query builder timedimesions * Query builder member changes * changes to queryBuilder * implement slots on query builder as well * remove overloaded props from renderer * change slot for empty but loaded meta * fix duplicate dependency issue and granularity to members * update docs * mantain full backwards compatibility * use only default slot if no other slot is present and add extra props * remove classes
2 participants
Since Vue slots are useful to show different parts and states I'm leveraging on a better solution to how queries are loaded. Also covered some areas with unit testing