Skip to content
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

small fixes and new props #22

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

Conversation

bgadrian
Copy link

@bgadrian bgadrian commented Apr 8, 2017

  • added ignore for .idea;
  • fixed css missing spaces tag title; (when a tag had spaces "no mine", and if the search term was "no" or "mi" the space dissapeared.
  • added prop to add custom classes to main input;
  • added addNew optional property for each category; I made a "custom keywords" category and let the user only add there, this way the "add new ..." will noe appear 10 times if you have 10 categories.
  • removed duplicate field from README
  • added prop inputAutoSize to control the size attribute of the main input ; if the size is controlled with CSS example bootstrap, using the "size" mess the layout.

…p to add custom classes to main input; added addNew optinal property for each category; removed duplicate field from README
Copy link
Owner

@erizocosmico erizocosmico left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Good job.

Only a couple of small requests and LGTM.

README.md Outdated
@@ -64,6 +66,7 @@ The category object for the dataset looks like this:
title: 'Title displayed on the category row',
items: ['Array', 'With', 'Tags'],
single: optional boolean. If is true the row will be treated as one-valued row. It does not have the option of adding new items to the category
addNew: optional boolean. Explicity allow or dissalow for a category to receive new tags, from the user.
Copy link
Owner

Choose a reason for hiding this comment

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

s/dissalow/disallow/

src/Input.jsx Outdated
this.props.value.length;
let extraProps = {};

if (this.props.inputAutoSize){
Copy link
Owner

Choose a reason for hiding this comment

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

space between ) and {

src/Input.jsx Outdated
onFocus={this.props.openPanel} onBlur={this.onBlur}
onChange={this.props.onValueChange} onKeyDown={this.props.onKeyDown}
placeholder={placeholder} aria-label={placeholder}
className='cti__input__input' />
className={this.props.inputClass === undefined ? 'cti__input__input' : this.props.inputClass} />
Copy link
Owner

Choose a reason for hiding this comment

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

s/this.props.inputClass === undefined/!this.props.inputClass/

@@ -21,7 +21,7 @@ const Panel = React.createClass({
<Category key={c.id} items={c.items} category={c.id} title={c.title}
selected={this.props.selection.category === i}
selectedItem={this.props.selection.item}
input={this.props.input} addNew={this.props.addNew}
input={this.props.input} addNew={c.addNew===undefined ? this.props.addNew : !!c.addNew}
Copy link
Owner

Choose a reason for hiding this comment

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

s/c.addNew === undefined/!c.addNew/

src/Input.jsx Outdated
let extraProps = {};

if (this.props.inputAutoSize){
extraProps["size"] = (this.props.value.length === 0 ?
Copy link
Owner

Choose a reason for hiding this comment

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

since there are no more extraProps right now, and I don't know if there will be in the future, what do you think about moving this to a size variable which, by default is null and set it directly as size={size} (which won't be rendered when it's null)?

Copy link
Author

Choose a reason for hiding this comment

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

You are right, with null is better, I forgot that react is so smart. I would like to add other props, for example "maxlength" is a must, for me at least it ruins CSS if you type or paste a large text, when using the "size" attribute.

@bgadrian
Copy link
Author

I've done the changes you ask, and by mistake I pushed a new feature, I hope is ok 8f8789b

It was weird as in the user sees "Fruits" category but can't see all the fruits available, Instead of making a helper page to list all the tags, the search itself can be be used to discover what tags are available. I have over 100 tags so it's a real problem for me.

@erizocosmico
Copy link
Owner

👍 for the changes.

About the new feature, I think it's okay, but I'd rather have it display all the category tags only if there is no tag that matches the input. Otherwise, if the category has the same name as a tag, it's difficult to select that tag.

@bgadrian
Copy link
Author

Makes more sense to show all of them by default, trough sometimes will require a scroll. I do not know how to remove a commit from the pull request though. The chance that a category has the same title as a tag is kinda low if I think about it.

I made other small features too, if this works maybe I'll make another push request next week. Example to change the minimum input length (now is hard coded 1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants