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

Trigger an event when a color is selected #415

Open
szily20 opened this Issue Jan 9, 2017 · 11 comments

Comments

Projects
None yet
2 participants
@szily20
Contributor

szily20 commented Jan 9, 2017

When a color is selected in the Color component, it should fire an event(similar to the scroller's onChange event).

@szily20 szily20 added the feature label Jan 9, 2017

@stehlo

This comment has been minimized.

stehlo commented Jan 25, 2017

I have now examined this solution (which is very far from the simple scenario I have proposed), and I can conclude that it is not very useful.

We need to consider that without building a further elaborate logic, we still won't know what are the selected colours.

As I don't have time to wait for a new solution now, I am going to use these intermediary new event handlers, but just to give an idea what is the result of using onItemTap() method:

  • Select the first colour. You get some index and value and selected=true.
  • Select the second colour. You get some index and value and selected=true.
  • Click one of those two colours again. You get some index and value and selected=true.
  • Click the same colour again. You get some index and value and selected=false.

Now, which colour was deselected?

Well, you won't know unless you have been calculating this information yourself.

That's the reason why I asked for a return of the whole object of selected and unselected colours in my original request.

The new onItemTap() and onPreviewItemTap() methods are nearly useless without the above information.

@stehlo

This comment has been minimized.

stehlo commented Jan 25, 2017

Just to make sure that my statement is understood properly:
"selected and unselected colours" means the resulting object of the multiple select (including the information about the "skipped" spots), as opposed to the non-sparse array that is being currently returned for "multiple select" option.

@stehlo

This comment has been minimized.

stehlo commented Jan 25, 2017

Just to add:
Frankly, I don't see currently any point for even having the onPreviewItemTap() method.
I still can't figure out any scenario where this could have been used in a sensible way.

@stehlo

This comment has been minimized.

stehlo commented Jan 25, 2017

However, I need to add that I truly grateful for this solution anyway, because it allows for the resolution of the original scenario – albeit with further workarounds.
Thank you.

@stehlo

This comment has been minimized.

stehlo commented Jan 25, 2017

Well, while it wasn't exactly difficult, it is also not very straight-forward. Please see below:

var previewColors        = [],
    previewColorsIndices = [];

...
onItemTap: function (event) {
    if (event.selected) {    // some color was selected
        if (typeof previewColors[0] === "undefined") {
            previewColors[0] = event.value;
            previewColorsIndices[0] = event.index;
        } else if (typeof previewColors[1] === "undefined") {
            previewColors[1] = event.value;
            previewColorsIndices[1] = event.index;
        }
    } else {    // some color was unselected
        if (previewColorsIndices[0] === event.index) {
            previewColors[0] = "";
            previewColorsIndices[0] = "";
        } else if (previewColorsIndices[1] === event.index) {
            previewColors[1] = "";
            previewColorsIndices[1] = "";
        }
    }
    now_use_the_colours_if_any(previewColors[0], previewColors[1]);
},
...

And that's just for two colours, i.e. multiple "select: 2" only!!!

The more colours are used the more complex the workaround will become.

We need to realise that now the check needs to happen using the indices and not the colour values as I initially thought – because the colour can actually be changed before deselecting(!!!).

Don't forget this when dealing with these new methods!

@stehlo

This comment has been minimized.

stehlo commented Jan 25, 2017

OK, there is indeed a problem present in this.

The refine slider's activity doesn't trigger the onItemTap() event handler when refining the currently selected colour.

Only here I can see how onPreviewItemTap() can be used – but that's just to the rescue for the missing feature. However, the workaround code is becoming seriously complicated now...

@stehlo

This comment has been minimized.

stehlo commented Jan 25, 2017

What is odd is the discrepancy between output values (of events' properties)...

Indices of onItemTap() event object are strings.

And indices of onPreviewItemTap() event object are numbers.

@stehlo

This comment has been minimized.

stehlo commented Jan 25, 2017

OK, when looking at the following code, please realise that it is still just for 2 colours only ("multiple" select: 2).

var initialPreviewColor  = "initial",
    previewColors        = [initialPreviewColor, initialPreviewColor],
    previewColorsIndices = [];

// set the preview colors to the initial ones
populateTimersFontSizeAndColors(timerId, isFavorite, initialPreviewColor, initialPreviewColor);
coloriseEnhancedTimer(timerId, isFavorite, initialPreviewColor);

...
onItemTap: function (event) {
    if (event.selected) {    // some color was selected
        if (previewColors[0] === initialPreviewColor && previewColorsIndices[1] !== event.index) {
            previewColors[0] = event.value;
            previewColorsIndices[0] = event.index;
        } else if (previewColors[1] === initialPreviewColor && previewColorsIndices[0] !== event.index) {
            previewColors[1] = event.value;
            previewColorsIndices[1] = event.index;
        } else if (previewColorsIndices[0] === event.index) {
            previewColors[0] = event.value;
        } else if (previewColorsIndices[1] === event.index) {
            previewColors[1] = event.value;
        }
    } else {    // some color was unselected
        if (previewColorsIndices[0] === event.index) {
            previewColors[0] = initialPreviewColor;
            previewColorsIndices[0] = "";
        } else if (previewColorsIndices[1] === event.index) {
            previewColors[1] = initialPreviewColor;
            previewColorsIndices[1] = "";
        }
    }
    populateTimersFontSizeAndColors(timerId, isFavorite, previewColors[0], previewColors[1]);
    coloriseEnhancedTimer(timerId, isFavorite, previewColors[0]);
},
onPreviewItemTap: function (event) {
    if (event.value) {    // some color is defined
        previewColors[event.index] = event.value;
    } else {    // some color is undefined
        previewColors[event.index] = initialPreviewColor;
    }
    populateTimersFontSizeAndColors(timerId, isFavorite, previewColors[0], previewColors[1]);
    coloriseEnhancedTimer(timerId, isFavorite, previewColors[0]);
},
...

Well, as you can see, this is becoming crazy.

It works, but it still doesn't behave as it should. The "refine" option's onChange() events are really missing.

It is absolutely clear that my original proposal to trigger onChange() event with a complete object of "[un]selected colours" would have been a much cleaner and simpler solution.

(I have uploaded the latest built in case you wished to see this in action. Ask Isti for the URL.)

@stehlo

This comment has been minimized.

stehlo commented Jan 25, 2017

In case you can't understand why the above code became so complicated, then you need realise that it is possible to click on an already selected colour and it will still trigger the onItemTap() event handler, yet you can move the refine slider and/or you can click the colour after the deselecting of the other one, so you can by mistake take it for the second one and not the first one. It's really mad.

Can you imagine doing this for six colour selection, which needs to be ordered precisely?

You need to seriously re-think this component as it feels like a half-baked one right at this moment.
It is OK for the simplest possible uses (but that's something that happens only in an ideal world).

@stehlo

This comment has been minimized.

stehlo commented Jan 26, 2017

I am just adding one last, immensely important point:

  • The most important objective, i.e. to know what colour spots were filled in by the user, is still not fulfilled.
  • When the user presses the "Set" button, the system still do NOT know which colours are being provided.
  • The return array is still non-sparse(!).
@stehlo

This comment has been minimized.

stehlo commented Jan 26, 2017

My conclusion is that these two methods utterly missed the target. They brought about more problems than solutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment