Improve border properties support #279
Conversation
Minor adjustments to building the internal border-info property, and then convert View to use that property and the data therein to draw the border. We build a separate path for each border edge and draw these individually so a separate colour can be applied to each. Unfortunately anti-aliasing at the end of a path drawn by JUCE means that it looks like there's a small gap between lines. I don't have a good solution for that at present.
Dotted borders seem to work as intended, but rendering of dashed borders is less certain. In both cases, changing the dash distances can result in bad rendering. I suspect a JUCE issue.
…a width or height.
There was a problem hiding this comment.
Thanks @banburybill! On the whole looks quite nice. I left a couple comments below, and I wonder as well if it'd be worth pulling some of the cpp code out of View.cpp? Just to keep the code well organized. What do you think?
| @@ -32,6 +104,12 @@ export class ViewInstance { | |||
| constructor(id: string, type: string, props?: any, parent?: ViewInstance) { | |||
| this._id = id; | |||
| this._type = type; | |||
| this._border = { | |||
There was a problem hiding this comment.
No need to carry this state on the ViewInstance, let's have parseBorderProp return a value which looks like this
There was a problem hiding this comment.
(Sorry for delay returning to this)
I could be not understanding something here, but don't we need to carry state around in the ViewInstance? There's no longer necessarily a single border property, but potentially a series of border- properties that may cumulatively specify what actually needs to be drawn. Surely we need to accumulate that state somewhere?
There was a problem hiding this comment.
Hmm. Guess we could have parseBorder(Side)Prop() take a border object and return a transmuted one. Still need to have something accumulating the values, though.
There was a problem hiding this comment.
Ah, I see what you mean. We do need to accumulate these property sets so that we have an accurate cumulative specification, but that will happen underneath the javascript layer at the appropriate View, no?
I'm trying to read through my notes and previous discussion here, because I can totally imagine that I may have said we should do the the accumulation here in js so that we can pass a single object prop, and now I might just be contradicting myself... but I don't see that past conversation.
Perhaps, then, that's a worthwhile question to ask here: does it help us to accumulate here in javascript rather than transform the incoming border props onto perhaps multiple native props? i.e. we can accumulate here and then setProperty("border", accumulatedBorderObject) as shown in this PR, or we can receive a directive to set the "border-radius" property to "4px 8px" and map that onto a series of setProperty calls: setProperty("border-radius-left", 4px); setProperty("border-radius-right", 4px).. etc.
|
|
||
| // Look for border properties and translate into our internal | ||
| // border-info property. | ||
| if (propKey.startsWith("border")) { |
There was a problem hiding this comment.
I think we can probably extract a border property match/normalize utility out of this. We did something like that for colors originally, but after a bit of searching through RN's source I found this: https://github.com/facebook/react-native/tree/1465c8f3874cdee8c325ab4a4916fda0b3e43bdb/packages/normalize-color. I think we should probably replace our own color parser with their normalize-color and try to refactor the code below into our own normalize-border package. What do you think?
There was a problem hiding this comment.
Using normalize-color would certainly improve colour specification flexibility. No harm in putting border parsing into its own module.
…ties This fixes borders not being drawn when only the individual side colours were specified.
Placate prettier.
Break border drawing into separate drawing of top/right/bottom/left border components and add properties for border-(top|right|bottom|left)-(color|style|width) and border-(top-left|top-right|bottom-right|bottom-left)-radius. Also add styles 'dotted' and 'dashed'.
Fixes #143