Skip to content

Improve border properties support #279

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Improve border properties support #279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5193350
Initial version of update border drawing.
Jul 28, 2021
f4ccf61
Add implementation of dotted and dashed borders.
Jul 28, 2021
ae09281
Draw each border as a single path.
Aug 2, 2021
6748280
Ensure corner horizontal/vertical radiuses do not exceed half the are…
Aug 2, 2021
bf6d406
Add border-(top|bottom|left|right)-color to the list of colour proper…
Sep 21, 2021
8b63578
[build] Fix breakages in View.cpp due to failed conversions double->f…
dstephenson-inmusicbrands Sep 20, 2021
dcffb7d
Code style fixups and add explicit BorderInfo type
Sep 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
184 changes: 184 additions & 0 deletions packages/react-juce/src/lib/Backend.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,18 @@ import SyntheticEvents, {
} from "./SyntheticEvents";
import { macroPropertyGetters } from "./MacroProperties";
import Colors from "./MacroProperties/Colors";
import invariant from "invariant";

//TODO: Keep this union or introduce a common base class ViewInstanceBase?
export type Instance = ViewInstance | RawTextViewInstance;

type BorderInfo = {
width: number[];
radius: string[];
color: string[];
style: string[];
};

let __rootViewInstance: ViewInstance | null = null;
let __viewRegistry: Map<string, Instance> = new Map<string, Instance>();
let __lastMouseDownViewId: string | null = null;
Expand All @@ -22,16 +30,98 @@ const cssPropsMap = allCssProps
.filter((s) => !s.startsWith("-") && s.includes("-"))
.reduce((acc, v) => Object.assign(acc, { [camelCase(v)]: v }), {});

// known CSS border styles for React Native
const cssBorderStyles = ["dotted", "dashed", "solid"];

// Parse combination border properties.
function parseBorderSideProp(
name: string,
val: string | number,
info: string[] | number[]
) {
invariant(
typeof val === "string" || typeof val === "number",
name + " must be a string or a number"
);

if (typeof val === "number") {
info[0] = info[1] = info[2] = info[3] = +val;
return;
}

const values = val.split(" ");

invariant(
values.length >= 1 && values.length <= 4,
"border-" + name + " should be a space separated string with 1 to 4 values."
);

switch (values.length) {
case 1:
info[0] = info[1] = info[2] = info[3] = values[0];
break;
case 2:
info[0] = info[2] = values[0];
info[1] = info[3] = values[1];
break;
case 3:
info[0] = values[0];
info[1] = info[3] = values[1];
info[2] = values[2];
break;
default:
info[0] = values[0];
info[1] = values[1];
info[2] = values[2];
info[3] = values[3];
}
}

function parseBorderProp(val: string, info: object) {
/*
Parameters can be in any order. So we need to recognised
which might be which.
*/
const numbers = "0123456789.-+";
const values = val.split(" ");

invariant(
values.length >= 1 && values.length <= 3,
"border should be a space separated string with 1 to 3 values."
);

const bs = info["style"];
const bw = info["width"];
const bc = info["color"];

for (val of values) {
if (cssBorderStyles.includes(val)) {
bs[0] = bs[1] = bs[2] = bs[3] = val;
} else if (numbers.includes(val.charAt(0))) {
bw[0] = bw[1] = bw[2] = bw[3] = +val;
} else {
bc[0] = bc[1] = bc[2] = bc[3] = val;
}
}
}

export class ViewInstance {
private _id: string;
private _type: string;
private _border: BorderInfo;
public _children: Instance[];
public _props: any = null;
public _parent: any = null;

constructor(id: string, type: string, props?: any, parent?: ViewInstance) {
this._id = id;
this._type = type;
this._border = {
Copy link
Collaborator

@nick-thompson nick-thompson Aug 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to carry this state on the ViewInstance, let's have parseBorderProp return a value which looks like this

Copy link
Author

@banburybill banburybill Sep 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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?

Copy link
Author

@banburybill banburybill Sep 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@nick-thompson nick-thompson Sep 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

width: [0, 0, 0, 0],
radius: ["0", "0", "0", "0"],
color: ["", "", "", ""],
style: ["solid", "solid", "solid", "solid"],
};
this._children = [];
this._props = props;
this._parent = parent;
Expand Down Expand Up @@ -144,6 +234,100 @@ export class ViewInstance {
NativeMethods.setViewProperty(this._id, k, v);
return;
}

// Look for border properties and translate into our internal
// border-info property.
if (propKey.startsWith("border")) {
Copy link
Collaborator

@nick-thompson nick-thompson Aug 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@banburybill banburybill Sep 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using normalize-color would certainly improve colour specification flexibility. No harm in putting border parsing into its own module.

let gotBorderProp: boolean = true;

switch (propKey) {
case "border":
parseBorderProp(value, this._border);
break;

case "border-color":
parseBorderSideProp("color", value, this._border["color"]);
break;
case "border-top-color":
this._border["color"][0] = value;
break;
case "border-right-color":
this._border["color"][1] = value;
break;
case "border-bottom-color":
this._border["color"][2] = value;
break;
case "border-left-color":
this._border["color"][3] = value;
break;

case "border-radius":
parseBorderSideProp("radius", value, this._border["radius"]);
break;
case "border-top-left-radius":
this._border["radius"][0] = value;
break;
case "border-top-right-radius":
this._border["radius"][1] = value;
break;
case "border-bottom-right-radius":
this._border["radius"][2] = value;
break;
case "border-bottom-left-radius":
this._border["radius"][3] = value;
break;

case "border-style":
parseBorderSideProp("style", value, this._border["style"]);
break;
case "border-top-style":
this._border["style"][0] = value;
break;
case "border-right-style":
this._border["style"][1] = value;
break;
case "border-bottom-style":
this._border["style"][2] = value;
break;
case "border-left-style":
this._border["style"][3] = value;
break;

case "border-width":
parseBorderSideProp("width", value, this._border["width"]);
break;
case "border-top-width":
this._border["width"][0] = value;
break;
case "border-right-width":
this._border["width"][1] = value;
break;
case "border-bottom-width":
this._border["width"][2] = value;
break;
case "border-left-width":
this._border["width"][3] = value;
break;

default:
gotBorderProp = false;
break;
}

invariant(
cssBorderStyles.includes(this._border["style"][0]) &&
cssBorderStyles.includes(this._border["style"][1]) &&
cssBorderStyles.includes(this._border["style"][2]) &&
cssBorderStyles.includes(this._border["style"][3]),
"unknown border-style."
);

if (gotBorderProp) {
propKey = "border-info";
value = this._border;
}
}

//@ts-ignore
return NativeMethods.setViewProperty(
this._id,
Expand Down
10 changes: 9 additions & 1 deletion packages/react-juce/src/lib/MacroProperties/Colors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
import ColorString from "color-string";
import ColorNames from "color-name";

const COLOR_PROPERTIES = ["border-color", "background-color", "color"];
const COLOR_PROPERTIES = [
"border-color",
"border-top-color",
"border-bottom-color",
"border-left-color",
"border-right-color",
"background-color",
"color",
];

const isColorProperty = (propKey: string): boolean => {
return COLOR_PROPERTIES.includes(propKey);
Expand Down
Loading