Skip to content

Feat/new chips input #1681

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.

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

Merged
merged 18 commits into from
Nov 28, 2021
Merged

Feat/new chips input #1681

merged 18 commits into from
Nov 28, 2021

Conversation

ethanshar
Copy link
Collaborator

@ethanshar ethanshar commented Nov 19, 2021

Description

Reimplement new ChipsInput based on Incubator.TextField (under Incubator)

Changelog

New ChipsInput component based on our new TextField implementation

Copy link
Contributor

@lidord-wix lidord-wix left a comment

Choose a reason for hiding this comment

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

Looks great, but seems like we remove some features, is it intended?
Also, there're some commented lines that I think we can remove..

Comment on lines 46 to 48
const styles = StyleSheet.create({
container: {}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be removed :)

Comment on lines 17 to 19
chips?: ChipProps[];
defaultChipProps?: ChipProps;
onChange?: (chips: ChipProps[], changeReason: ChipsInputChangeReason, updatedChip: ChipProps) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add descriptions for these props as well

key={index}
customValue={index}
// resetSpacings
// paddingH-s2
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I kept it for now so I'll have a quick reference in case there is something missing while we're migrating.
I added a TODO

@ethanshar ethanshar added the Important for Next Release PR that must be included in the release version label Nov 26, 2021
@ethanshar ethanshar requested a review from lidord-wix November 28, 2021 07:24
Copy link
Contributor

@lidord-wix lidord-wix left a comment

Choose a reason for hiding this comment

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

Cool,
There're few redundant imports, so you can remove them and merge :)

Comment on lines 1 to 4
import _ from 'lodash';
import React, {Component} from 'react';
import {StyleSheet} from 'react-native';
import {View, Text, Card, TextField, Button, Colors, Incubator} from 'react-native-ui-lib'; //eslint-disable-line
Copy link
Contributor

Choose a reason for hiding this comment

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

There're few redundant imports here..

@ethanshar ethanshar enabled auto-merge (squash) November 28, 2021 10:20
@ethanshar ethanshar merged commit 2bea3df into master Nov 28, 2021
@ethanshar ethanshar deleted the feat/NewChipsInput branch June 13, 2022 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Important for Next Release PR that must be included in the release version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants