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.

Sign up for GitHub

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

Jump to bottom

Migrate the Task Description Card #321

Merged
merged 17 commits into from
Dec 11, 2020
Merged

Migrate the Task Description Card #321

merged 17 commits into from
Dec 11, 2020

Conversation

macite
Copy link
Member

@macite macite commented Dec 9, 2020 •
edited
Loading

Description

This PR will migrate the task-description-card to Angular.

From:
image

To:
image

Buttons appear as:
image

@macite macite force-pushed the migrate/task-description-card branch from 81b3849 to b2ed31b Compare December 10, 2020 00:49
@macite macite force-pushed the migrate/task-description-card branch from b2ed31b to eaa7e6e Compare December 10, 2020 01:13
@macite macite marked this pull request as ready for review December 10, 2020 01:22
@macite macite requested a review from jakerenzella December 10, 2020 01:22
@macite macite changed the base branch from development to inbox-redesign December 10, 2020 01:29
@macite macite changed the base branch from inbox-redesign to development December 10, 2020 01:29
@macite
Copy link
Member Author

macite commented Dec 10, 2020

@jakerenzella this PR is ready to review... Let me know what you think of the styling. Also the use of File Saver to allow the download of the resources from a button action rather than opening on a separate page. This could also be used when we move the auth tokens to the header.

jakerenzella
jakerenzella requested changes Dec 11, 2020
View reviewed changes
Copy link
Member

@jakerenzella jakerenzella left a comment

Choose a reason for hiding this comment

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

Some thoughts on styling: I think the styling needs to be worked on a little, especially the clarity and location of the buttons, but this PR is acceptable to go through and can be worked on in the future

The components need some small changes, really just the icon fixe, but otherwise looks good.

Regarding the guide: Looks good. I have some additions about tooling that can go into another PR so I'll make an issue for now

...ectives/task-dashboard/directives/task-description-card/task-description-card.component.html Outdated Show resolved Hide resolved
...ectives/task-dashboard/directives/task-description-card/task-description-card.component.html Outdated Show resolved Hide resolved
...irectives/task-dashboard/directives/task-description-card/task-description-card.component.ts Outdated Show resolved Hide resolved
@macite
Copy link
Member Author

macite commented Dec 11, 2020

I also removed the "Download" from the buttons, and the "Key Information" sub title. What do you think @jakerenzella?

image

jakerenzella
jakerenzella approved these changes Dec 11, 2020
View reviewed changes
Copy link
Member

@jakerenzella jakerenzella left a comment

Choose a reason for hiding this comment

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

Looks good! I think in the future we need to slate a discussion around a consistent styling of action buttons but this is good for now 👍

@macite macite merged commit b70faa4 into doubtfire-lms:development Dec 11, 2020
@macite macite deleted the migrate/task-description-card branch December 11, 2020 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakerenzella jakerenzella jakerenzella approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@macite @jakerenzella