Color picker - popup update

[Rdornier]

Hello,

The new color picker by default pops up every time we click on a recent color, which is a bit annoying. I simplly call the popup opening when the user explicitely clicks on the picker.

Rémy.

[will-moore]

Hi, thanks for the PR. I agree that it's not necessary to show the color-picker every time a user chooses a recent color, and it's handy to auto-pick the "new-color" (at the top-right) when you pick a recent color, since that saves you having to interact with the color-picker.

However, I think it's still useful to show the color-picker when a user first opens the Color Picker dialog, otherwise it's not obvious what to click on to choose a new color, given the rather sparse dialog:

Also, since the user has chosen a More colors... menu to launch the dialog, it's likely they want to choose a new color so we save them an extra click by already showing the color-picker.

[Rdornier]

Hi Will,

Thanks for the feedback

Also, since the user has chosen a More colors... menu to launch the dialog, it's likely they want to choose a new color so we save them an extra click by already showing the color-picker.

Ok, you're right. I updated the code to make appear the dialog if there is no previous picked colors. In case there are some, it is not automatically shown, as they might also want to use one of the pre-selected colors.

otherwise it's not obvious what to click on to choose a new color, given the rather sparse dialog:

True again. I added some labels to make it more clear.

Rémy.

[will-moore]

Thanks Rémy,

I had another look at this, and I'm afraid I really think that the browser colour-picker should always be visible as it is before this PR (I find that it's more useful than annoying): Even if you have already got some previously-chosen colours, it's still quite likely that you'll want to use the colour-picker to choose new colours. And even when you click on one of your previously-chosen colours, it still makes sense to show the colour-picker with that colour, in case you decide that you actually want to tweak it a bit and go a bit ligher/darker etc.

We still need the labels for Selected color and Previous color that you've added, and the fix for updating of the selected colour when you click on a previous colour is also nice, but can we leave out the other changes to the colour-picker behaviour? (and we probably don't need the Open color picker label either if we keep it always open, since it nudges the colour-picker outside the dialog area):

This looks great:

Sorry, thanks.

[Rdornier]

Hi Will,

Thanks for your feedback.

It tooks me a while to understand why you were not enthusiastic with the changes... but now I see. I'm using Firefox as a default browser (and it is same for the computers in out institute) and I realize that the color picker in Firefox doesn't have the same behavior from the one on Edge or Chrome (I didn't tested yet on Sarfari).

Here is how it behaves on Firefox

As you can see, the current behavior is annoying as it open a popup window that prevents from editing the omero colorpicker until it's closed. The proposed update changes the Color setting from 3(4) clicks to 2 and streamlines the process for a Firefox user. But from Edge/Chrome point of view, the changes in picker visibility don't make sense.

Would it be ok for you if I try to introduce the changes only if the detected browser is Firefox ?

[will-moore]

Ah, I see. Sorry, I should have thought of that. I can see how that is pretty annoying.

Yes, see if you can detect the browser and how well that works. I've not had to do browser detection in a long time, but that might be the best solution here.

[Rdornier]

Ok, no worries. I found out how to do browser detection. I also added a label for the recent colors.

[will-moore]

Looks good. I think you originally had it so that you never showed the color-picker when the dialog was opened, even if you have no previous colours? I think that still makes sense with Firefox. Also you can remove call of $(".color-input", this.$el); on it's own since that does nothing.

I think a longer label is nicer, and if it's below the input then it's more out of the way for when we're automatically showing the picker (and you don't need the label). Sorry, maybe being a little picky there! Just dumping my current diff...

$ git diff
diff --git a/src/index.html b/src/index.html
index 31261ba4..d6bf243b 100644
--- a/src/index.html
+++ b/src/index.html
@@ -630,8 +630,8 @@
           </div>
           <form class="colorpickerForm" role="form">
             <div class="modal-body" style="height: 300px">
-              <div>Picker</div>
               <input class="color-input" type="color" />
+              <div>Click to Open color-picker</div>
               <div
                 id="demo_cont"
                 class="demo demo-auto inl-bl"
diff --git a/src/js/views/colorpicker.js b/src/js/views/colorpicker.js
index e32e1560..7ac55a7b 100644
--- a/src/js/views/colorpicker.js
+++ b/src/js/views/colorpicker.js
@@ -50,10 +50,10 @@ var ColorPickerView = Backbone.View.extend({
     // 'Recent colors' buttons have color as their title
     pickRecentColor: function(event) {
         var color = $(event.target).prop('title');
-        if(navigator.userAgent.includes("Firefox")){
-            $('.color-input').val(color);
-        }else{
-            $(".color-input").val(color).trigger("click");
+        $('.color-input').val(color);
+        // Only show color picker if not Firefox
+        if (!navigator.userAgent.includes("Firefox")){
+            $('.color-input').trigger("click");
         }
         $('.oldNewColors li:first-child').css('background-color', color);
         // enable submit
@@ -89,9 +89,8 @@ var ColorPickerView = Backbone.View.extend({
     show: function(options) {
         showModal("colorpickerModal");

-        if(this.pickedColors.length > 0 && navigator.userAgent.includes("Firefox")){
-            $(".color-input", this.$el);
-        }else{
+        // Only show color picker if not Firefox
+        if(!navigator.userAgent.includes("Firefox")){
             $(".color-input", this.$el).trigger("click");
         }

[Rdornier]

Thanks for your feedback, @will-moore

I think a longer label is nicer, and if it's below the input then it's more out of the way for when we're automatically showing the picker (and you don't need the label).

Ok, it makes sense

I think you originally had it so that you never showed the color-picker when the dialog was opened, even if you have no previous colours?

In my very first commit, yes. But, then, I modified it to show the color picker if there is no recent colors. The reason is that it saves one click for the first time someone choose a new color. I think it's still nice to have it.

[will-moore]

Looks good and working fine now, thanks.

[will-moore]

This is now released in OMERO.figure 7.1.0