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

Components: Adding a generic token field component #924

Merged
merged 5 commits into from
May 31, 2017

Conversation

youknowriad
Copy link
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 jasmussen and aduth May 29, 2017 14:23
@nylen
Copy link
Member

nylen commented May 29, 2017

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
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
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
Contributor Author

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

Copy link
Member

@aduth aduth left a comment

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.

@@ -30,6 +30,11 @@ $blue-medium-300: #66C6E4;
$blue-medium-200: #BFE7F3;
$blue-medium-100: #E5F5FA;

// Alerts
$alert-yellow: #f0b849;
Copy link
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
Copy link
Member

aduth commented May 31, 2017

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
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
Copy link
Member

aduth commented May 31, 2017

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
Copy link
Member

nylen commented Jun 1, 2017

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