-
Notifications
You must be signed in to change notification settings - Fork 13
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
add colour picker for intro title, subtitle and button #508
base: main
Are you sure you want to change the base?
Conversation
ee7f95c
to
a35b9bc
Compare
Your demo site is ready! 🚀 Visit it here: https://ramp4-pcar4.github.io/storylines-editor/fix-506 |
a35b9bc
to
4acec50
Compare
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.
Nice functionality! Overall, it looks very nice.
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @RyanCoulsonCA)
src/components/helpers/metadata-content.vue
line 199 at r1 (raw file):
/> </div> <div class="metadata-item">
May be worth explaining somewhere that the 'Button' refers to the "Jump to storyline contents" (or whatever it's called) button.
src/components/helpers/colour-picker-input.vue
line 10 at r1 (raw file):
:style="{ 'background-color': selectedColour }" :class="!disabled ? 'cursor-pointer' : ''" class="flex flex-1 rounded w-10 mr-1"
Minor nitpick: Either make the color picker's right corners unrounded (change rounded
to rounded-l
), or add some slight padding on all sides (or just left + top + bottom) of the parent button. Either one would visually integrate the color picker into the button slightly better.
src/components/helpers/colour-picker-input.vue
line 25 at r1 (raw file):
<span v-if="isOpen" class="text-white mx-auto self-center font-bold mix-blend-difference">X</span> </div> <input
It's a bit hard to tell that the hex text is selectable/modifiable. Maybe add a slight gray background upon hover?
Also, it seems that if you enter a non-valid hex value here, nothing happens. The user might erroneously think it's worked, or be confused why the text changed but not the color in the picker. I'd suggest either some sort of error message ("This is not a valid hex color"), or to revert the text to the last valid hex value.
792fe03
to
eeba2fd
Compare
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.
Reviewable status: 6 of 9 files reviewed, 3 unresolved discussions (waiting on @gordlin)
src/components/helpers/colour-picker-input.vue
line 10 at r1 (raw file):
Previously, gordlin (Gordon Lin) wrote…
Minor nitpick: Either make the color picker's right corners unrounded (change
rounded
torounded-l
), or add some slight padding on all sides (or just left + top + bottom) of the parent button. Either one would visually integrate the color picker into the button slightly better.
Donethanks.
src/components/helpers/colour-picker-input.vue
line 25 at r1 (raw file):
Previously, gordlin (Gordon Lin) wrote…
It's a bit hard to tell that the hex text is selectable/modifiable. Maybe add a slight gray background upon hover?
Also, it seems that if you enter a non-valid hex value here, nothing happens. The user might erroneously think it's worked, or be confused why the text changed but not the color in the picker. I'd suggest either some sort of error message ("This is not a valid hex color"), or to revert the text to the last valid hex value.
Added the slight background change on hover.
src/components/helpers/metadata-content.vue
line 199 at r1 (raw file):
Previously, gordlin (Gordon Lin) wrote…
May be worth explaining somewhere that the 'Button' refers to the "Jump to storyline contents" (or whatever it's called) button.
Good catch, I've added a little info icon to clarify.
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.
Reviewed 6 of 9 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gordlin)
eeba2fd
to
79c2ba2
Compare
Rebase added some unexpected changes for some reason. Working on it now. |
False alarm. All should be good. I blame RTO 🏢 |
79c2ba2
to
b00eaac
Compare
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.
Reviewed 1 of 9 files at r1, 1 of 3 files at r2, 6 of 7 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gordlin)
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.
Reviewed 1 of 3 files at r2, 6 of 7 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)
Related Item(s)
#506, #494
Changes
Notes
Open to feedback on the design and functionality!
Testing
Steps:
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)