Add ability to select coloscheme for spectrogram #142
Conversation
naglepuff
force-pushed
the
issue-133-alt-color-scheme
branch
from
4a6ab84 to
426f473
Compare
naglepuff
force-pushed
the
issue-133-alt-color-scheme
branch
from
426f473 to
5223263
Compare
There was a problem hiding this comment.
Just a few suggestions:
- Remove Alpha selection from background color picker
- Maybe add a default background color to go along with the color scheme
- Suggestion on representation of the color scheme in the color scheme picker through a custom component
- Swapping watch effect to a more explicit watch
| @@ -48,6 +48,7 @@ export default defineComponent({ | |||
| blackBackground, | |||
| scaledVals, | |||
| configuration, | |||
| colorScheme, | |||
There was a problem hiding this comment.
probably used in testing but doesn't seem to be used in this field anymore?
| const gValues = ref(''); | ||
| const bValues = ref(''); | ||
|
|
||
| watchEffect(() => { |
There was a problem hiding this comment.
Could this be switched to a more explicit watch (I know it may have a few variables it is watching, such as backgroundColor and colorScheme)? I've just been bitten in the past by watchEffect getting large and making it difficult to determine the triggering variable/function.
Below is just an explanation of why I have an aversion to watchEffect ,but I'm open to have my mind changed:
It was a more complicated instance with other issues, but the watchEffect made it difficult to track the logic down. A codebase I was being onboarded on had a function that modified a ref, that function was passed a prop into a child component that was renamed and then used in a large (~200 line watchEffect). The chaining and the name changes made it difficult for me to determine why and where the variable was changing.
client/src/views/Spectrogram.vue
Outdated
| <v-color-picker | ||
| v-model="backgroundColor" | ||
| elevation="0" | ||
| /> |
There was a problem hiding this comment.
add mode='hex' or mode='rgb' to the attributes on this component. It will remove the alpha portion of the selector. Really isn't needed because I don't think it would apply to the background color.
| <v-select | ||
| v-model="colorScheme" | ||
| label="Color Scheme" | ||
| :items="colorSchemes" | ||
| item-title="title" | ||
| item-value="scheme" | ||
| variant="outlined" | ||
| density="compact" | ||
| width="75" | ||
| hide-details | ||
| return-object | ||
| /> |
There was a problem hiding this comment.
I think we may want to include an indication of the color scheme mostly because I don't know off hand what those names mean.
Something like this:
I included a component that can be used as a base (it's a text file because github doesn't like .vue files). It's just a custom V-select that provides a visual indication of the color scheme. Feel free to modify anything in it (I did the highlighting and border stuff quick for the individual items).
I think this or something like this should replace the configuration color picker as well.
| default_color_scheme = models.CharField( | ||
| max_length=20, | ||
| choices=AvailableColorScheme.choices, | ||
| default=AvailableColorScheme.INFERNO, | ||
| ) |
There was a problem hiding this comment.
If you're adding a default color scheme should there be a default background color as well?
naglepuff
force-pushed
the
issue-133-alt-color-scheme
branch
from
bc844ca to
9e6c1ff
Compare
naglepuff
force-pushed
the
issue-133-alt-color-scheme
branch
from
9e6c1ff to
310df37
Compare
Fix #133
Adds the ability to select a color scheme to apply to the spectrogram on the front end. The "0" value can be set as well (defaults to black). Also adds the ability to change the default color scheme on the Admin page.