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 color picker display when updating an environment color #2711

Merged

Conversation

jgiovaresco
Copy link
Contributor

@jgiovaresco jgiovaresco commented Oct 11, 2020

We were removing the color picker to reset event handlers easily (fix #811).
It is possible to remove the listener quite easily by listening to the change event triggered when the color picker modal is closed.

There was still an issue with the 1st click on Change color. We were adding the input dynamically on the 1st click and we were having some race condition between the click and the appendChild 🤔
To fix that, I've added the input element just before the Color dropdown. Besides, now we can open the color picker modal near the dropdown by making the input element invisible.

Before
image

After
color-picker

Closes #2647

develohpanda
develohpanda previously approved these changes Oct 19, 2020
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Seems alright but I'd like to know how it was working before the Electron update or if the logic here changed recently.

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

I've noticed that for any reason calling await this._load(this.state.workspace) just after appending the input element to the body is fixing the display of the color picker.

I'm guessing that adding an await to something after appending the input element, is allowing some background process to complete.

I doesn't feel like forcibly updating the environment color each time we prompt for a new color is fixing the root cause.

I was able to get the same fix working by delaying the element click using setTimeout(() => el.click(), 50);, so needs further investigation to determine and fix the root cause.

@develohpanda develohpanda dismissed their stale review October 19, 2020 21:14

Needs further investigation as always updating the environment colour is fixing a symptom and not the root cause.

@jgiovaresco jgiovaresco force-pushed the fix/2647-environment-color-selection branch 2 times, most recently from 2f84384 to a5aa16a Compare October 20, 2020 17:16
@jgiovaresco jgiovaresco force-pushed the fix/2647-environment-color-selection branch from a5aa16a to 10787e3 Compare October 20, 2020 17:30
@jgiovaresco jgiovaresco force-pushed the fix/2647-environment-color-selection branch 2 times, most recently from 84d441d to 9af80e8 Compare October 21, 2020 05:40
@jgiovaresco jgiovaresco force-pushed the fix/2647-environment-color-selection branch from 9af80e8 to 232bfde Compare October 21, 2020 18:17
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Thanks for updating the description and adding in a gif. Almost there!

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Nicely done 👏

@develohpanda develohpanda merged commit 5852d27 into Kong:develop Oct 27, 2020
@jgiovaresco jgiovaresco deleted the fix/2647-environment-color-selection branch October 27, 2020 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Environment color selection on MacOS [BUG] Changing environment colour affects multiple environments
3 participants