FEAT: Create TagsInput component #177

FilipMessa · 2019-01-07T07:26:59Z

No description provided.

netlify · 2019-01-09T12:53:51Z

Deploy preview for kiwicom-universal-components ready!

Built with commit 0bc5c1c

https://deploy-preview-177--kiwicom-universal-components.netlify.com

FilipMessa · 2019-01-09T16:36:17Z

Finally, the MR is ready, it was tricky find way how to handle it with one code because of differences between web input and native input.
Especially on the web input element, is no easy change width base on a count of characters.

I created the hidden element (sizer) and based on it I count the input width. It is kind of hacky but I can't find a better solution.

So pls check it properly I spent a lot of the time on it so maby I missed something. Thanks

src/TagsInput/components/Label.js

src/TagsInput/components/IconButton.js

src/TagsInput/TagsInput.js

FilipMessa · 2019-01-11T13:08:13Z

I realize that there is the issue in test

FilipMessa · 2019-01-11T13:21:14Z

Issue with test fixed

JosefDuda · 2019-01-11T13:49:31Z

ios/storybooknative.xcodeproj/project.pbxproj

@@ -1085,6 +1106,13 @@
 				5C04BEA7D09C4DC4B05BB661 /* Roboto-MediumItalic.ttf in Resources */,
 				90151CF9607749A28BB60FDD /* Roboto-Regular.ttf in Resources */,
 				F9A904A78F284EE6989903AF /* orbit-icons.ttf in Resources */,
+				D88FDFB084E3435482711AB1 /* orbit-icons.ttf in Resources */,


Do you think this is related to your recent tests fix update? I'm just wondering why all fonts were added to xcode project for a second time.

I'll check it

Yes, could you try to remove this file from the commit and see if you can still run the storybook?

I am working on it

src/Badge/Badge.js

RobinCsl · 2019-01-11T14:08:27Z

src/TagsInput/TagsInput.js

+};
+
+class TagInput extends React.Component<Props, State> {
+  inputRef: any; // @TODO find a way how to type this properly, the issue with mixing react and react-native


I think we are using react > v16.3, so that could be helpful: https://reactjs.org/docs/refs-and-the-dom.html#creating-refs

This was the first approach what I tried but, it didn't work.

RobinCsl · 2019-01-11T14:09:48Z

src/TagsInput/TagsInput.js

+    if (containerWidth && width) {
+      return containerWidth - width;
+    }
+    return undefined;


I've always been told that it was better to return null than undefined, as null meant value could not be set instead of being undefined.

OK, just there is another side of the coin, https://youtu.be/rhV6hlL_wMc?t=1326 😄
But I will change it as you suggest.

Thanks for the reference, I think he's got some very valid points. If we feel strongly about this, then we should enforce it.

RobinCsl · 2019-01-11T14:10:02Z

src/TagsInput/TagsInput.js

+  getWidth = (width: ?number) => {
+    const { containerWidth } = this.state;
+    if (containerWidth && width) {
+      return containerWidth - width;


What if you get a negative result?

It can't happen I suppose, in the current use-case.
It can be zero but not negative.

src/TagsInput/TagsInput.js

src/TagsInput/components/DeleteButton.js

src/TagsInput/components/InputField.js

RobinCsl · 2019-01-11T14:19:34Z

src/TagsInput/components/TagsContainer.js

+    }
+
+    return (
+      <View>


Is this wrapping necessary?

It is, I need it for the web, if I don't use it break how overflow works

RobinCsl · 2019-01-11T14:20:04Z

src/TagsInput/components/TagsContainer.js

+          style={[styles.container, styles.row, { maxWidth }]}
+          showsHorizontalScrollIndicator={false}
+          contentContainerStyle={{
+            alignItems: 'center',


Could you extract the style?

I tried before but I get back this flow error,

Then we might need to address how our StyleSheet.create works, compared to RN's native one. Or use StyleSheet.create from RN implementation.
@tbergq What do you think about this issue?

OK, I played with the flow for while but without success so I decided to use StyleSheet.create from RN implementation.

RobinCsl · 2019-01-15T09:12:22Z

src/TagsInput/TagsInput.js

+    // $FlowFixMe property focus is missing in object type
+    Platform.OS === 'web' && this.inputRef.current.focus();
+
+    if (onClearPress) {


I believe you can even do onClearPress?.().

thanks, I have been learned something new.

RobinCsl · 2019-01-15T09:14:05Z

src/TagsInput/TagsInput.js

+            fontSize={fontSize}
+            disabled={disabled}
+            minWidth={MIN_WIDTH}
+            maxWidth={this.getWidth(MIN_WIDTH)}


Maybe the naming could be improved here: why is maxWidth linked to MIN_WIDTH?

Because of in the getWidth method I subtract passing param, in this case, MIN_WIDTH from the width of the whole container.
And because TagsContainer minWidth is also MIN_WIDTH, maxWidth of the InputField must respect this.

I will try to improve it to something more readable.

RobinCsl · 2019-01-15T09:17:07Z

src/TagsInput/components/InputField.js

+|};
+
+type State = {|
+  componentWidth: number | string,


Suggested change

componentWidth: number | string,

componentWidth: number | 'auto',

That's the only string that's allowed, right?

I cannot see it added now?

RobinCsl · 2019-01-15T09:18:45Z

src/TagsInput/components/InputField.js

+  },
+  input: {
+    web: {
+      outline: 'none',


Can this be a problem for accessibility?

I've improved website design. An inspiration I took from kiwi.com

src/TagsInput/components/InputField.js

Summary: Created TagsInput component. Added tests and Storybook's stories for the TagsInput.

FilipMessa force-pushed the placePicker/tags-input branch 10 times, most recently from 34cc212 to 18de489 Compare January 8, 2019 15:08

FilipMessa force-pushed the placePicker/tags-input branch 3 times, most recently from 4ffe572 to 33c3f73 Compare January 9, 2019 16:25

FilipMessa changed the title ~~WIP: Create TagsInput component~~ FEAT: Create TagsInput component Jan 9, 2019

FilipMessa force-pushed the placePicker/tags-input branch from 33c3f73 to add74b7 Compare January 10, 2019 15:42

JosefDuda reviewed Jan 10, 2019

View reviewed changes

src/TagsInput/components/Label.js Outdated Show resolved Hide resolved

src/TagsInput/components/IconButton.js Outdated Show resolved Hide resolved

src/TagsInput/TagsInput.js Outdated Show resolved Hide resolved

JosefDuda reviewed Jan 10, 2019

View reviewed changes

src/TagsInput/TagsInput.js Outdated Show resolved Hide resolved

FilipMessa force-pushed the placePicker/tags-input branch from add74b7 to 1ac1014 Compare January 11, 2019 13:06

FilipMessa force-pushed the placePicker/tags-input branch from 1ac1014 to 5ceae12 Compare January 11, 2019 13:19

JosefDuda reviewed Jan 11, 2019

View reviewed changes