Updates from mdl-list to flexbox for display of variable controls. Closes #86. #87

chriscox · 2017-03-28T15:23:38Z

No description provided.

codecov-io · 2017-03-28T15:24:51Z

Codecov Report

Merging #87 into develop will increase coverage by 0.24%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           develop     #87      +/-   ##
==========================================
+ Coverage    81.95%   82.2%   +0.24%     
==========================================
  Files           18      19       +1     
  Lines          643     652       +9     
  Branches        79      80       +1     
==========================================
+ Hits           527     536       +9     
  Misses          85      85              
  Partials        31      31

Impacted Files	Coverage Δ
src/lib/Constants.ts	`100% <ø> (ø)`	⬆️
src/ui/controls/ColorSwatchControl.tsx	`77.77% <ø> (ø)`	⬆️
src/ui/controls/SliderControl.tsx	`80.95% <ø> (+0.95%)`	⬆️
src/ui/controls/TextFieldControl.tsx	`75% <ø> (+1.08%)`	⬆️
src/ui/controls/DropdownControl.tsx	`82.6% <ø> (+0.79%)`	⬆️
src/ui/controls/SwitchControl.tsx	`74.07% <0%> (+0.99%)`	⬆️
src/ui/controls/RadioListControl.tsx	`71.42% <0%> (ø)`	⬆️
src/ui/ListItem.tsx	`100% <100%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 673bc71...0b55301. Read the comment docs.

chriscox · 2017-03-28T15:50:24Z

Hiya @appsforartists looking for sanity check here. No new functionality added, but really just a UI refactor. Essentially this creates a new ListItem component with params used for placement of child properties. This is first of a few cleanup changes I'm making to rid of mdl-list and mdl-list-item in favor of a more structured and reusable representation.

appsforartists · 2017-03-29T22:24:10Z

src/lib/Constants.ts

@@ -64,6 +64,7 @@ export const DataType = {

 /** CSS class and id constants. */
 export const CSS = {
+  // TODO(cjcox): Refactor out these MDL classes in favor of flexbox.
  MDL_LIST:               "mdl-list",


It's Google Style to prefer single quotes.

I was in the habit of doing doubles before working here, but have switched to singles after reading that. Ultimately, it's more important to be consistent within your project than adhere to that style guide, but being consistent (both within Material and with Google as a whole) is worth some consideration too. 😃

Thanks. I filed an issue where I'll handle this for entire app.

appsforartists · 2017-03-29T22:26:22Z

src/ui/ListItem.tsx

+ * Interface for the properties assigned to the ListItem component.
+ * @interface
+ */
+interface IListItemProps {


Similarly, I think Google Style (and TypeScript convention) is to omit the I prefix. If you've done it other places, you can do it here, but it may be worth doing a pass where you move this project's conventions closer to Google Style.

Thanks. I filed an issue where I'll handle this for entire app.

appsforartists · 2017-03-29T22:28:10Z

src/ui/ListItem.tsx

+export function ListItem(props: IListItemProps) {
+  let inline = props.inlineControl ? "inline" : "";
+  return (
+    <div className={`${CSS.RMX_LIST_ITEM} ${props.controlClass} ${inline}`}>


Your spacing within braces doesn't seem consistent: some pairs have them and some don't. I usually include it, but consistency is the most important.

I'll take a pass with linter in subsequent PR to fix here and throughout.

appsforartists · 2017-03-29T22:31:39Z

src/ui/controls/ColorSwatchControl.tsx

+            <ColorSwatch
+              color={ColorUtils.toRgbaString(value)}
+              key={value}
+              isSelected={ColorUtils.areEqual(selectedValue, value)}


I hope areEqual is a reasonably fast comparison - since it's in render, it will be called for every swatch on every render.

If it's at all expensive, it may be worth calculating it when the values change and caching the result.

It is a cheap string comparison.

appsforartists · 2017-03-29T22:38:06Z

src/ui/ListItem.tsx

+  subtitle?: string;
+  inlineControl: boolean;
+  controlClass?: string;
+  control: any;


It might be better to use children instead of control. Then, your markup would look like this:

<ListItem title = { title } subtitle = { title } > <ColorSwatch /> </ListItem>

instead of

<ListItem title = { title } subtitle = { title } control = { <ColorSwatch /> } />

Ultimately, doesn't matter - it's just style. It just looks a bit weird to see markup passed as a prop.

Fixed here and throughout. Now setting the control code a child.

chriscox

Made updates per review.

chriscox · 2017-03-30T00:28:05Z

src/lib/Constants.ts

@@ -64,6 +64,7 @@ export const DataType = {

 /** CSS class and id constants. */
 export const CSS = {
+  // TODO(cjcox): Refactor out these MDL classes in favor of flexbox.
  MDL_LIST:               "mdl-list",


Thanks. I filed an issue where I'll handle this for entire app.

chriscox · 2017-03-30T00:29:45Z

src/ui/ListItem.tsx

+ * Interface for the properties assigned to the ListItem component.
+ * @interface
+ */
+interface IListItemProps {


Thanks. I filed an issue where I'll handle this for entire app.

chriscox · 2017-03-30T00:46:27Z

src/ui/ListItem.tsx

+  subtitle?: string;
+  inlineControl: boolean;
+  controlClass?: string;
+  control: any;


Fixed here and throughout. Now setting the control code a child.

chriscox · 2017-03-30T00:52:32Z

src/ui/ListItem.tsx

+export function ListItem(props: IListItemProps) {
+  let inline = props.inlineControl ? "inline" : "";
+  return (
+    <div className={`${CSS.RMX_LIST_ITEM} ${props.controlClass} ${inline}`}>


I'll take a pass with linter in subsequent PR to fix here and throughout.

chriscox · 2017-03-30T00:56:00Z

src/ui/controls/ColorSwatchControl.tsx

+            <ColorSwatch
+              color={ColorUtils.toRgbaString(value)}
+              key={value}
+              isSelected={ColorUtils.areEqual(selectedValue, value)}


It is a cheap string comparison.

appsforartists · 2017-03-30T01:06:43Z

I have a tslint.json in Material Motion. Feel free to copy it. 😃

https://github.com/material-motion/material-motion-js/blob/develop/tslint.json

Though it doesn't enforce spacing in braces.

chriscox added 4 commits March 27, 2017 15:43

Initial commit of new UI layout.

8a3e654

Progress checkin.

0c7bbfb

Progress checkin.

641f4a1

Progress checkin.

2ed0804

chriscox added 2 commits March 28, 2017 08:25

Updates constants.

77d7b5b

Adds comment.

c0f09f5

chriscox requested review from chuga and appsforartists and removed request for chuga March 28, 2017 15:29

Removes unused selector.

b7fdb0d

appsforartists approved these changes Mar 29, 2017

View reviewed changes

Updates per review.

0b55301

chriscox commented Mar 30, 2017

View reviewed changes

chriscox merged commit 540a1a6 into develop Mar 30, 2017

chriscox deleted the feature/ui_refactor branch March 30, 2017 01:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates from mdl-list to flexbox for display of variable controls. Closes #86. #87

Updates from mdl-list to flexbox for display of variable controls. Closes #86. #87

chriscox commented Mar 28, 2017

codecov-io commented Mar 28, 2017 •

edited

Loading

chriscox commented Mar 28, 2017

appsforartists Mar 29, 2017

chriscox Mar 30, 2017

appsforartists Mar 29, 2017

chriscox Mar 30, 2017

appsforartists Mar 29, 2017

chriscox Mar 30, 2017

appsforartists Mar 29, 2017

chriscox Mar 30, 2017

appsforartists Mar 29, 2017

chriscox Mar 30, 2017

chriscox left a comment

chriscox Mar 30, 2017

chriscox Mar 30, 2017

chriscox Mar 30, 2017

chriscox Mar 30, 2017

chriscox Mar 30, 2017

appsforartists commented Mar 30, 2017

Updates from mdl-list to flexbox for display of variable controls. Closes #86. #87

Updates from mdl-list to flexbox for display of variable controls. Closes #86. #87

Conversation

chriscox commented Mar 28, 2017

codecov-io commented Mar 28, 2017 • edited Loading

Codecov Report

chriscox commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appsforartists commented Mar 30, 2017

codecov-io commented Mar 28, 2017 •

edited

Loading