New keyboard shortcuts + tooltips refactoring #519
Conversation
src/devtools/env.js
Outdated
| @@ -0,0 +1,17 @@ | |||
| import Vue from 'vue' | |||
|
|
|||
| // Env | |||
There was a problem hiding this comment.
Now since you moved this code to a separate file this comment seems redundant
src/devtools/locales/en.js
Outdated
| }, | ||
| EventsHistory: { | ||
| filter: { | ||
| tooltip: '[F] To filter on components, type <span class="input-example"><i class="material-icons">search</i> <MyComponent></span> or just <span class="input-example"><i class="material-icons">search</i> <mycomp</span>.' |
There was a problem hiding this comment.
HTML with classes in translations make it much harder to maintain code dependent on it, you never know which change might affect it. I'd be careful about this
| @@ -24,6 +24,8 @@ const mutations = { | |||
| state.inspectedIndex = -1 | |||
| }, | |||
| 'INSPECT' (state, index) { | |||
| if (index < 0) index = 0 | |||
| if (index >= state.events.length) index = state.events.length - 1 | |||
There was a problem hiding this comment.
wouldn't this be enough actually?
state.inspectedIndex = Math.max(0, state.events.length - 1)
There was a problem hiding this comment.
Ah, true. It would have to be:
Math.min(Math.max(0, index), state.events.length - 1)
but it wouldn't be necessarily more readable ;D
src/devtools/plugins.js
Outdated
| keys | ||
| }, | ||
| replacer: text => { | ||
| text = text.replace(keyboardReg, '<span class="keyboard">$1</span>') |
There was a problem hiding this comment.
I don't like this solution. Now whenever [] or || appears somewhere in translation, it's going to wrap this part with one of those elements. As for the icon - if it's a generic app-wide solution I'm ok with it, but probably with better interpolation signs. e.g <<iconName>>, or at least ||iconName|| - though the first one more readable I guess.
But keyboard is rather case specific thing, and I wouldn't put it here. I'm thinking about alternative ways, and perhaps it would be better to make a dedicated file just for tooltips content, and import keys explicitly there.
That would leave us with translations in tact, without extra concerns. WDYT?
There was a problem hiding this comment.
We are going to have a lot of keyboard shortcuts in the strings--more than icons 😛
|
Here is a summary of the keyboard shortcuts: https://docs.google.com/spreadsheets/d/1iv3cBE1BpW8mRZLZQDWDbXMmncscmplZ6tJaYocue48/edit?usp=sharing |
For example, modifier 'ctrl+alt' isn't dispatch if 'shift' is pressed too.
|
The order of modifiers is:
They are joined together with a Examples of valid modifiers: '' // No modifier
'ctrl'
'ctrl+shift'
'ctrl+alt'
'ctrl+shift+alt'Examples of invalid modifiers: null
'ctrl-alt'
'shift+ctrl'
'alt+shift+ctrl' |
There was a problem hiding this comment.
It looks like Alt+C and Alt+E do not work on Mac, they must be reserved or something.
BTW, you may want to use https://github.com/shentao/vue-global-events for handling global events 😄
|
@posva Changed to Ctrl|Meta + 1..3 |
There was a problem hiding this comment.
One last comment from me, other than that it looks really good. Great job @Akryum :)
src/devtools/mixins/entry-list.js
Outdated
| inspectedIndex (value) { | ||
| this.$nextTick(() => { | ||
| const el = value === -1 ? this.$refs.baseEntry : this.$refs.entries[value] | ||
| el && scrollIntoView(document.querySelector('.left .scroll'), el, false) |
There was a problem hiding this comment.
I recommend we use .js- prefixed classes for everything that is being handled by JS. It would allow us to more safely deal with the code and potential refactors.
|
Ha, realized later that Cmd+numbers may not be that good because On another topic, I think the |
Tested on MacBook and working even with Dvorak layout. Just don't press Shift key 😛 |
$ttranslation pluginWe may want to move all texts to the locale file. Also, this open up translation for a future PR.