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

feat(tasks): new task card layout #893

Merged
merged 13 commits into from
Mar 5, 2019

Conversation

alexkrolick
Copy link
Contributor

@alexkrolick alexkrolick commented Feb 27, 2019

Changes are feature-flagged as new features are added for the new API

New

screen shot 2019-02-26 at 6 19 10 pm

Old
screen shot 2019-02-26 at 6 18 22 pm

@alexkrolick alexkrolick requested a review from a team as a code owner February 27, 2019 02:02
@alexkrolick alexkrolick requested a review from a team February 27, 2019 02:02
@alexkrolick alexkrolick requested a review from a team as a code owner February 27, 2019 02:02
@boxcla
Copy link

boxcla commented Feb 27, 2019

Verified that @alexkrolick has signed the CLA. Thanks for the pull request!

package.json Outdated
@@ -167,8 +170,8 @@
"cypress": "^3.1.5",
"deepmerge": "^2.1.1",
"draft-js": "^0.10.1",
"enzyme": "^3.6.0",
"enzyme-adapter-react-16": "^1.5.0",
"enzyme": "3.8.0",
Copy link
Contributor Author

@alexkrolick alexkrolick Feb 27, 2019

Choose a reason for hiding this comment

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

Latest is 3.9.0 but there is a new bug that affected one of our tests: enzymejs/enzyme#2020

@alexkrolick alexkrolick force-pushed the feat/tasks/new-api branch 2 times, most recently from 2206d78 to 101d6f1 Compare February 27, 2019 02:08
@priyajeet
Copy link
Contributor

priyajeet commented Feb 27, 2019

Can this PR be broken into multiple small ones

  1. API changes
  2. UI Changes
  3. Other things like changes into package.json/base styles/things not related directly to tasks
  4. feature checking feature improvements

@alexkrolick
Copy link
Contributor Author

Will rebase after the sub-PRs

@alexkrolick alexkrolick changed the title feat(tasks): new api feat(tasks): new task card layout Mar 4, 2019
taskType?: TaskType,
};

const TaskActions = ({ onTaskApproval, onTaskReject, onTaskComplete, taskType }: Props): React.Node => (
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it may be a duplicate of the other TaskActions component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what other taskactions?

}
};

const StatusMessage = ({ status }: { status: TaskStatus }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually try to stick to one component per file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like not exporting the components would make it difficult to test them in isolation. @priyajeet, thoughts?

jstoffan
jstoffan previously approved these changes Mar 5, 2019
jstoffan
jstoffan previously approved these changes Mar 5, 2019
- this is feature-flagged under
  activityFeed.tasks.newApi
- tasks are created and read from new service when enabled
- the new API format is not compatible with the old one
- there are 538 warnings for this
- do we still need --max-warnings=0
  or are we happy with the error/warning ratio now?
@alexkrolick alexkrolick merged commit f664803 into box:master Mar 5, 2019
@alexkrolick alexkrolick deleted the feat/tasks/new-api branch March 5, 2019 02:24
@ConradJChan
Copy link
Contributor

🎉 This PR is included in version 10.0.0-beta.6 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@jstoffan
Copy link
Contributor

🎉 This PR is included in version 10.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

6 participants