-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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: setup devtools integration #1949
Conversation
after closing/re-opening the devtools
@kiaking I've moved the functions from |
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.
AWESOME WORK GUYS!!!! 🎉 🎉 🎉 🎉 🎉
@Akryum I left 1 comment, I wasn't 100% sure (unused color vars).
Also, what do you think about positioning actions on top of mutations in Timeline view. It's really a personal preference but for me it kinda feels more natural to have actions timeline on top.
Especially when actions are async, having timeline range bar on top of mutations dots might look better...? Again, just my personal preference though 😳
src/plugins/devtool.js
Outdated
/** | ||
* Extracted from tailwind palette | ||
*/ | ||
const COLOR_PINK_500 = 0xec4899 |
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.
Should we remove other than
const COLOR_LIME_500 = 0x22d3ee
const COLOR_DARK = 0x666666
const COLOR_WHITE = 0xffffff
Since seems like we're not using them...?
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.
This might be my fault lol
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.
Oh OK cool! I'll remove it then 👍 I wasn't sure if Devtools was doing some ultra magic to pick these colors up some how haha
Pushed small code style change and fix linter error. Lint error is still there due to #1949 (comment). Not sure why CI is not picking it up 🤔 |
package.json
Outdated
@@ -54,6 +54,9 @@ | |||
"peerDependencies": { | |||
"vue": "^3.0.2" | |||
}, | |||
"dependencies": { | |||
"@vue/devtools-api": "^6.0.0-beta.7" |
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.
If it is added as a dependency, shouldn't we mark it as external too? I will need to follow the same on Vue Router. Currently it is not marked as external and therefor is only a dev dependency
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 this was here to support Type imports in TS...? In that case I think it should be the same in Router too. Am I correct @Akryum ?
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.
It does nothing except calling a function or putting the plugin into a queue array.
I don't know, I also thought about it but then the actions layer can grow its height where the mutations will not. So it will probably be pushed far at the bottom if there are a lot of parallel actions going on. |
@Akryum Ah good point. Nice catch. In that case yeah it make sense to have actions under mutations 👍 |
Closes #1942