Add different colours for drawings

[Jonas-July]

Currently, there are only the 3 modes "Erase", "Rainbow" and "Black" to draw with. For writing that leaves only a black pen. This PR introduces the possibility to choose different colours to draw with. Works on #65

While allowing for arbitrary colour selection, default colours could also be provided in order to improve usability.

Possibly work on the rainbow logic.

[Jonas-July]

I now tried using the color input type as suggested. It works somewhat fine, though the icon/appearance doesn't change colour, so one cannot see which colour was chosen (if any at all) before drawing. The issue seems to be that the frame isn't updated; Even directly updating the colour value in the element doesn't change the appearance. Calling this.render() directly also doesn't seem to work.

[NickGeek]

@Jonas-July

I now tried using the color input type as suggested. It works somewhat fine, though the icon/appearance doesn't change colour, so one cannot see which colour was chosen (if any at all) before drawing. The issue seems to be that the frame isn't updated; Even directly updating the colour value in the element doesn't change the appearance.

Updating a field won't cause the component to re-render. If you need the component to re-render based on internal state, you need to use this.setState(...) (see https://reactjs.org/docs/react-component.html#setstate). Although I don't know if I'd advise going down that route because re-rendering the component is a bit risky. I think the reason the drawing component is so janky is because the canvas gets wiped on a re-render.

One easy alternative is to use an uncontrolled form component (see https://reactjs.org/docs/uncontrolled-components.html).

So: <input type="color" defaultValue={this.drawColour} onChange={e => this.drawColour = e.target.value} /> would make it an uncontrolled component (defaultValue vs. value).

Calling this.render() directly also doesn't seem to work.

The render function should never be directly called, that's React's job. render is (generally) called when the props change and/or the state of the component changes. The drawing component doesn't use any react-managed state so it only re-renders when the props change.

[NickGeek]

@Jonas-July Would you like a hand making that change?

[Jonas-July]

I was a bit busy, sorry for that. Thanks for the reminder though.

The solution using an uncontrolled component works fine and as expected, so that is good. I have now removed the other options and the previously offending colours array.

I'd still like some form of default values or something, so that it's easier to select the same colours again after changing. Though the colour picker already does that sort of (one can use the pipette anywhere to use that colour, so just click on the previous line with that). Maybe a small hint for that somewhere?

Other than that, I think that the draft is ready for merging

[Jonas-July]

Actually, the same issue as with the colour picker arises with the checkboxes. When selecting a colour, the checkboxes should be deselected aswell. Simply changing value to defaultValue didn't do anything (except a warning that value is required). Do you have an idea how to make them uncontrolled too (that presumably being the best option).

[NickGeek]

@Jonas-July I don't think having checkboxes is the best UX in this case. If you look at the MDN docs for <input type="color"> (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/color), there is an attribute called list that can be used on all browsers except for Firefox (for now).

Example

<input type="color" list="colours-list" defaultValue={this.drawColour} onChange={e => this.drawColour = e.target.value} />
<datalist key="mp-drawing-colours" id="mp-drawing-colours">
    <option value="#FF6900">Orange</value>
</datalist>

https://user-images.githubusercontent.com/3462055/140599823-7da14602-aed2-48ae-b63f-38e62121f0cc.mp4

Do you have an idea how to make them uncontrolled too (that presumably being the best option).

That wouldn't actually be the best option if we wanted to connect the state of the colour switcher to the checkboxes. An uncontrolled component manages its own state, so it's fine for a self-contained thing but if it needs to interact with external state then it should be controlled. In MicroPad I use Redux to manage state, so the likely solution would be to add the state to the store. I think we can get away with just killing the checkboxes and replicating them with that datalist feature of the colour selector.

[Jonas-July]

Using the list was exactly how I wanted it to be. Thanks for the hint. The current defaults are black, white, and the colours of the rainbow mode.

Due to being of type "color", the options only permitted colour codes as value (not "Rainbow" or "Eraser"). Instead, I created a second list of type "text" with the options "Rainbow", "Eraser" and "Colour", with the last being the default. For simplicity, I took up the idea of different drawing modes, between which the user can choose. The boolean checks are replaced with equality checks against the mode.

The mode list is a bit fiddly though, as it only displays the options matching the current selection instead of all options. So on click, the selection is cleared with a placeholder to show all options. This only works reliably on the second click though, as the first one seems to still use the old values.

I have also thought about using the Select from react-select. It would technically solve the above issues, but I think it displays a bit weirdly, since e.g. the bibliography is drawn on top of the dropdown. I might do a commit for that shortly, so you can see what I mean.

[Jonas-July]

Obvious disadvantage of the whole concept with two lists: They are still separate. Practically speaking, this becomes an issue when choosing a colour, because the mode is changed to "Colour", but not (immediately) updated in the mode list. So we might have to do some active state management or the mode can only be changed using the mode list.

But the checkboxes are gone, so that's an advantage

[NickGeek]

@Jonas-July I think there should be some state management to disable the other list depending on a select:

1. Line
2. Eraser
3. Rainbow Line

If you want to have a go at wiring up the Redux state management for that (probably using EditorReducer) feel free. Otherwise, I'll take a look at the code and I can wire up the state management, especially because the editors are a bit complex to deal with right now because I wrote them very early on in the project. What do you think?

[Jonas-July]

@NickGeek So I've been looking at the Reducers and it certainly could be made to work, but I haven't quite figured out how to get access to the EditorReducer (oder Reducers in general) in order to actually change the state. Therefore I think that you can start wiring it up and I'm gonna see how it is done and possibly continue from there. It's probably nothing special or complicated, but you certainly know the project better than I do and can find the necessary code sections faster to implement the changes. Thanks a lot for your help

[NickGeek]

@Jonas-July If you're interested to see how it's done, I'd recommend looking at the MarkdownEditorContainer and the MarkdownEditorComponent. The container wraps the component and provides state from the store and actions as props.

There are simpler containers and components elsewhere you can look at too.

[NickGeek]

Also, the "Redux Dev Tools" Chrome/Firefox plugin is useful for seeing the current store state and seeing how actions are affecting it.

[NickGeek]

@Jonas-July I've updated next-dev to connect the drawing component to the store. Apologies for what likely will be a few merge conflicts you have to deal with.

The new container can pass in data from the store into the component's props and also functions that dispatch actions that update the props. For what you want, you'll probably want to:

1. Create an action that updates the current drawing colour in the store (see actions.ts and EditorReducer.ts)
2. Add a dispatcher to the drawing container so it can dispatch that action
3. Use value instead of defaultValue for the colour picker and get the current colour from the store
4. Dispatch the update action when the colour picker changes

https://github.com/MicroPad/MicroPad-Core/commit/6177161f5bcdb172fe56cc6cad7585cf64083c05 might shed some light (although a lot of stuff there is more weird and complex than it should have needed to be because I was cleaning up old code)

[Jonas-July]

@NickGeek Thanks for the changes, they helped a lot. I implemented most of the state management now

Unfortunately I found a bug, which is also present on next-dev: The new state management triggers an update to the drawing element. So when changing the drawMode, the image is reset and the drawings lost. I test for this bug by selecting "Rainbow", then drawing a bit and then selecting "Line". This also works with the eraser. The same happens when changing the line colour, but simply setting the mode to "Line" (if that was the previous value) doesn't reset the image. See also #386

Maybe you could take a look at what went wrong. In the meantime, I will add the update functionality so the select box changes its shown value.

[Jonas-July]

Ok, the select somehow doesn't update immediately the shown value when selecting a colour directly. I tried that by selecting "Rainbow", then choosing any colour other than black. The select still shows "Rainbow". I checked that the drawings "render" method is called (which it is) and that the props have the correct value (which they do). Upon selecting a different colour, the select also automatically updates for some reason and shows "Line".

I also tried that with a text input field whose value is {this.props.drawMode}. That field updates immediately. So I'm not sure what the cause is, other than that it's something specific to the select.

[NickGeek]

@Jonas-July I've pushed up a commit that should tie in your code to the store, feel free to take a look and if you're happy I think it's good to go

[Jonas-July]

@NickGeek I was actually able to integrate the store previously. Thank you for cleaning up the code, though.

When changing the colour, I would want to start drawing immediately, so I added that the mode is changed to 'Line' aswell when setting the DrawingLineColour. Maybe a different name would be more descriptive? For now, though, it should be fine.

The biggest issue I had (apart from the deleted image, which you fixed) concerned the select box that wouldn't immediately update the mode, but do so on the second colour change. I have now introduced a hack-ish solution that at least functionally solves the problem. It basically mimicks the second colour change by briefly switching to black and back. Experimentally, this only worked within the render function, so I've put it in there. To avoid an infinite loop of re-renders, the logic is conditional on a variable that is only set when a new colour was picked and unset once the hack completes. I have also extensively documented that 'fix'. I usually don't put large comments in the code, but I think it is necessary here to avoid other people (or ourselves) unwittingly messing with the logic, because it is quite counterintuitive. The fix shouldn't be necessary. When running the code, a warning is displayed due to the re-renders. Visually, everything still works and I haven't encountered any problems. The warning also occurs only once.

If you are fine living with the hack above, the code is good to go. I don't see any functional issues.

[Jonas-July]

I just tested the code in Firefox (v94) for the first time (I use Chromium myself, so I worked with that). Firefox does the colour picker so much better than Chromium, I have to give them that.

Other than that, everything worked exactly the same. The last issue with the select box also occurs in Firefox, but is fixed with the last commit just like Chromium.

That's now tested aswell. Just thought to share.

[NickGeek]

@Jonas-July Thanks for checking out those changes! The solution you have for changing that state works but isn't ideal. Now that you have connected the component to the store, you can have the logic that changes the drawing mode in the reducer. The component shouldn't need to know or care about that logic, the view and settings of the component should be driven by the state in the reducer.

I'd recommend changing the reducer handler for the set colour action to also update the mode in the state to Line.

[NickGeek]

@Jonas-July Ah, sorry I misunderstood. There seems to be a bug in the UI library because our render function is being called but the internal one for <Select> isn't. I set a key field on it which will help react know to re-render it. That means everything updates correctly and we don't need a hack.

Also if you ever do need to force a re-render, there's the this.forceUpdate() method that can be used. That being said, it shouldn't be needed in almost every case.

[Jonas-July]

@NickGeek Yeah, there seems to be some issue that we can only workaround at the moment.\ The key field fixes the issue completely. That's a way nicer solution than my terrible hack at any rate. Thanks for that tip!

Now I think the code is ready to be merged. I also tested on Chromium and Firefox again, both work wonderfully.

Thanks again for all your help working on this matter. I learned a great deal during this time.

[NickGeek]

@Jonas-July Thanks for all the work on this! If you remove the draft tag on this I'll approve it and merge it in.

[NickGeek]

That'll be on https://next.getmicropad.com when the CI is done, and it'll go out with v4 to https://web.getmicropad.com when v4 is stable.