Skip to content

Components: Adding a generic token field component - #924

Merged
youknowriad merged 5 commits into
masterfrom
add/token-field-component
May 31, 2017
Merged

youknowriad merged 5 commits into
masterfrom
add/token-field-component

Conversation

@youknowriad

Copy link
Copy Markdown
Contributor

In order to do #854 we need a generic Tags Input (with auto-complete...). Building such a component from scratch is not an easy task. So I shamelessly copied the component from Calypso and I adapted it to match our guidelines. It's a lot of code, so I'd appreciate help reviewing this component.

screen shot 2017-05-29 at 15 19 48

In order to test it. I'm using it in the sidebar with static values.
In a follow-up PR. I'll try to use it to update the post tags.

@youknowriad youknowriad added the General Interface Parts of the UI which don't fall neatly under other labels. label May 29, 2017
@youknowriad youknowriad self-assigned this May 29, 2017
@youknowriad
youknowriad requested review from aduth and jasmussen May 29, 2017 14:23
@nylen

nylen commented May 29, 2017

Copy link
Copy Markdown
Member

I do not think we should merge this without automated tests. Fortunately they already exist and we now have a PR to add component tests in this repository (#926).

Can we / should we remove some of the unneeded functionality here like a token's status?

@youknowriad

Copy link
Copy Markdown
Contributor Author

Can we / should we remove some of the unneeded functionality here like a token's status ?

Sure we can, I don't know if we should? Couldn't this be useful somewhere else?

@youknowriad
youknowriad force-pushed the add/token-field-component branch from c51f7fb to f33b026 Compare May 30, 2017 09:38
@youknowriad

Copy link
Copy Markdown
Contributor Author

Tests are failing on CI but passing locally but the error is weird.

@youknowriad
youknowriad force-pushed the add/token-field-component branch from f33b026 to aaa7398 Compare May 31, 2017 11:04
@youknowriad

Copy link
Copy Markdown
Contributor Author

Tests are passing again. I'll appreciate some 👀 here. Thanks

@aduth aduth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When opening Post Settings on this branch, the page goes blank. I don't see the same on master.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Odd spacing. Previous variables make no effort in aligning by space.

@aduth

aduth commented May 31, 2017 •

Copy link
Copy Markdown
Member

Oh maybe that's related to #957. Could do for a rebase then.

@youknowriad
youknowriad force-pushed the add/token-field-component branch from 71ff59c to 586aac8 Compare May 31, 2017 14:46
@youknowriad

Copy link
Copy Markdown
Contributor Author

@aduth Yep, it was related, should be fixed now.

@youknowriad
youknowriad merged commit dce3dd8 into master May 31, 2017
@youknowriad
youknowriad deleted the add/token-field-component branch May 31, 2017 17:00
@aduth

aduth commented May 31, 2017

Copy link
Copy Markdown
Member

I forgot how slow these tests were, but we're now seeing Travis timeouts in unrelated branches because they take too long. We can bump timeouts, but I'd almost rather remove the tests before allowing ourselves to succumb to accepting slow tests.

@nylen

nylen commented Jun 1, 2017

Copy link
Copy Markdown
Member

Even though these tests use enzyme, they look more like "integration tests" than "unit tests" and perhaps should not be run on every build. They might fit better in another category of tests (#939 (comment)). However, TokenField has a long history of weird bugs and these tests are pretty important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

General Interface Parts of the UI which don't fall neatly under other labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants