dhis2 / notes

:memo: Memos, proposals, agendas and meeting minutes
19 stars 8 forks source link

Information needed: Color picker component #173

Open cooper-joe opened 4 years ago

cooper-joe commented 4 years ago

Calling @dhis2/team-apps, ui-core needs a color picker component. This component is used in maintenance and all analytics apps that have color legend functionality.

I believe the current implementation is React Color. Does anyone have any experience with this library, or any opinons as to whether we could incorporate it as part as ui-core? Or, would we rather build from scratch?

I wanted to do a technical check before I start designing to ensure I know what is possible/achievable with this component. I imagine building a full featured color picker from scratch is a resource intensive activity.

I did look into native OS color pickers, but I am unsure if they are reliable? Does anyone have any experience here?

varl commented 4 years ago

Quick note: If we are pulling in an external library that would put the color-picker in ui-widgets.

ismay commented 4 years ago

I have no idea actually. Have never dealt with colorpickers in a webapp. It does work rather nicely for me on macos and chrome: https://jsfiddle.net/65j389f2/1/. On ios it doesn't work for me though (though caniuse says it should? 🤔).

The caniuse data is sort of ok I guess: https://caniuse.com/#feat=input-color

stale[bot] commented 4 years ago

Hi! Due to a lack of activity on this issue over time (7.776*10^9 ms and counting, to be precise) it seems to be stale. If there is no further progress on it, it will be closed automatically.

If this is still relevant, maybe there is something you can do to move it forward? For example provide further information in a comment, or supply a PR? Any activity on this issue will keep it open. Thanks! 🤖

cooper-joe commented 4 years ago

Color picker is definitely a component that is needed across several apps. I think, before I start defining specs, it would be worth someone looking into the current state of available solutions.

HendrikThePendric commented 4 years ago

I've looked into this very quickly and these are my findings:

I don't really know the complete specs, so it's hard to draw conclusions, but do at least think that in some scenarios we need to let the users choose between a limited set of colors. For this scenario, the color-input is not really suitable, so I am leaning towards building a custom implementation with a canvas.

Idea: we could have a component that accepts an image (or img src path) as a prop. This image would represent the colors a user is allowed to select. This way we could have one color picker that is suitable for any palette.

ismay commented 4 years ago

Considering that we don't need IE11 support, a native seems like a valid option.

I'd prefer that. If that's not a possibility I'd prefer an existing library (say https://github.com/Simonwep/pickr or something).

If a user needs to pick from a limited set of colors I'd recommend we go for an existing library as well. The above suggestion supports showing swatches and hiding the palette.

I'd be very hesitant to start implementing and maintaining something ourselves. Why don't we stick to react color for example?

HendrikThePendric commented 4 years ago

From examples I've seen in the platform, a native input wouldn't cover all use-cases, so my educated guess is that that's off the table.

So now we enter the discussion of DYI vs off-the-shelve...

As far as I've understood it, we try to shy away from external dependencies as much as possible because they are regarded as a security vulnerability. For complex stuff, such as the positioning engine, I think that adding an external dependency and the additional risk it may bring, might be justified.

My initial thoughts were that a color-picker would be very complex to build too. But after looking at the image+canvas solution above, I changed my mind. Building a color picker seems very straightforward.

IMO we should at least look into building it ourselves first. If there are many complexities and "things start getting out of hand", we can consider an external dependency.

This would be the same process we are going through with the positioning stuff, and I think that's a good approach.

ismay commented 4 years ago

From examples I've seen in the platform, a native input wouldn't cover all use-cases, so my educated guess is that that's off the table.

Ok, yeah I don't know what requirements we have. Are they not met by the native colorpicker?

we try to shy away from external dependencies as much as possible because they are regarded as a security vulnerability

I'm skeptical of the above argument. I don't want to pull you into a whole debate about whether using open source is more secure than writing it ourselves, but my position is that we are already firmly entrenched in using open source, and that we shouldn't add dependencies willy-nilly of course, but that we also should avoid reinventing the wheel whenever possible.

I trust the security of widely used, well maintained libraries simply because of the number of people using it, the automated alerts, etc. Plus, a properly maintained external dependency will likely have decent accessibility, browser support, etc. baked in.

IMO we should at least look into building it ourselves first. If there are many complexities and "things start getting out of hand", we can consider an external dependency. This would be the same process we are going through with the positioning stuff, and I think that's a good approach.

Personally, given time constraints, I'd prefer looking at it from a time perspective. Will using an external dependency save us time in the long run (whilst satisfying our design/engineering requirements)?

cooper-joe commented 4 years ago

Bring this topic back up for discussion with a draft spec of a color picker component.

Features:

This is obviously not a native color picker. This picker would, in theory, supply a color code back to the element that called it (e.g. a button, select, etc.). A component could be defined for this (e.g. a custom select called ColorDisplay), but I don't want to restrict this.

What do you all think about the feasibility of including this in ui-* with/without using external dependencies?

Edit: I imagine the color picker content would be presented inside a popover/popper component to handle the positioning and scrolling.

Double-ninja-edit: I've updated the component to include functionality to remove a selection.

HendrikThePendric commented 4 years ago

In an earlier comment I mentioned that having a custom (i.e. zero-deps, build it ourselves) implementation for the "palette" version of the color picker would be fairly easy. And I suggested using an input with type color for the "free-selection". Given that the "palette" and "free-selection" are being used together in the current design-specs, my assessment isn't correct anymore.

When looking at the specs, I am confused about one thing:

Normally in a color picker, you first select a color (via the slider UI thingy), and then you select a gradient it the square box above: Screenshot 2020-03-02 at 09 34 04

However, in our design you just see a plain square of that box, so you don't see a gradient. Screenshot 2020-03-02 at 09 37 20

As it currently looks, I don't think the square has any function. So, I am assuming it is supposed to be a gradient selector, and the design-specs are going to be updated....

Given the current specs and my assumption above, I would asses the situation as follows:

Now to connect the above to @cooper-joe's question:

What do you all think about the feasibility of including this in ui-* with/without using external dependencies?

Yes, I think this is feasible because we can leverage the power of the canvas. We'd need the following parts:

cooper-joe commented 4 years ago

As it currently looks, I don't think the square has any function. So, I am assuming it is supposed to be a gradient selector, and the design-specs are going to be updated....

@HendrikThePendric Yes, that is meant to be a gradient selector area. In fact, with Firefox on mac it is for me: image I'm not sure why it's not working on your end. I'm doing some relative/absolute positioning tricks to get this to work, I didn't test it fully with other browsers for this demo though.

Thanks for the overview and assessment, I've linked to it from the open JIRA ticket.

HendrikThePendric commented 4 years ago

To add to the above, I'll give a breakdown of the components I think we will/should end up with: ColorPicker => See current specs, but without Popover ColorPickerButton => A button showing a color + color-code (?) ColorPickerField => Label + ColorPickerButton + HelpText + ValidationText, all wrapped in a Field. When the ColorPickerButton is clicked, we render a ColorPicker in a Popper/Popover