Skip to content
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

Fix #4106: Showing color preview in Edit Profile #4212

Merged
merged 4 commits into from
Oct 8, 2019
Merged

Fix #4106: Showing color preview in Edit Profile #4212

merged 4 commits into from
Oct 8, 2019

Conversation

saurabhdaware
Copy link
Contributor

@saurabhdaware saurabhdaware commented Oct 3, 2019

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

Fixes #4106.

The current color-picker does not show a preview so it is hard to guess the color contrast. I added a preview (screenshot added below) which shows your selected colors.

Also, Added a <input type="color"> which makes it is easy to select the color.

Since there are other pages that depend on 'color-picker' class I did not change anything in colorPicker.js or any of the styles of current color-picker. I created a new file colorPreview.js and some new classes in html.

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Mobile

Mobile screenshot of color preview feature

Desktop

Desktop Screenshot of color preview feature

Recording

Recording of color preview feature

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

My first PR To DEV!
I was not very sure if I could do this or not since DEV's codebase is probably the largest codebase I've ever worked on but now I'm super proud of myself to complete this 🌻

Me showing this code to everyone :
Elsa showing magic to Anna

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 3, 2019
@CLAassistant
Copy link

CLAassistant commented Oct 3, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @saurabhdaware, this PR looks great! I played with it and it opens the system color picker on my Mac and I can see the DEV logo change in real time :)

I've added just a small request on grouping the initializing code.

app/javascript/packs/colorPreview.js Outdated Show resolved Hide resolved
@pr-triage pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 7, 2019
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Oct 7, 2019
@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 7, 2019
@rhymes rhymes self-requested a review October 7, 2019 13:28
@rhymes rhymes added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Oct 7, 2019
@rhymes
Copy link
Contributor

rhymes commented Oct 7, 2019

@saurabhdaware can you take a look at the code climate issues as well just in case? I think it's complaining that updateColorFields and updateTextFields are basically doing the same exact thing.

@saurabhdaware
Copy link
Contributor Author

Not sure why it is showing the issue in it. updateColorField() Updates the color fields to be in sync with text fields and I call it when text fields are changed. and updateTextField() updates text fields to be in sync with color fields and they are called when color input is changed. can't really think of any other way of doing it.

@rhymes
Copy link
Contributor

rhymes commented Oct 7, 2019

@saurabhdaware Let's leave it like this :)

for future reference: a possible way would be to pass the elements as arguments to the function and have only one function, since you're basically swapping the values between the colorField and the textField. That change is probably going to trigger the parameter reassignment error in eslint though.

@saurabhdaware
Copy link
Contributor Author

Cool, thank you @rhymes ! and yeah we can pass parameters and get around code climate I guess. This way just makes it easier for me to update the fields whenever necessary. I'll keep it in mind for the future though. thanks :D

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 7, 2019
@maestromac maestromac merged commit 81e3f65 into forem:master Oct 8, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad coloring on follow button
4 participants