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

[WIP] Revisions list on course modal #2289

Open
wants to merge 12 commits into
base: trunk
Choose a base branch
from
Open

[WIP] Revisions list on course modal #2289

wants to merge 12 commits into from

Conversation

drewvolz
Copy link
Collaborator

@drewvolz drewvolz commented Oct 7, 2018

Part of #946

This lists the revisions in a collapsed details/summary element.

flyfishing


Known Issues:

  • flow types
  • data types not taken into account (see CSCI 253A)
  • adapt a table-like diff (described below)
  • different fonts

@drewvolz
Copy link
Collaborator Author

drewvolz commented Oct 7, 2018

I'd like to iterate on this to make things cleaner and more componentized. Feedback would be much appreciated.

@drewvolz
Copy link
Collaborator Author

drewvolz commented Oct 7, 2018

Also, flow is about to yell.

modules/gob-web/modules/course/revisions.js Outdated Show resolved Hide resolved
return (
<ul key={index}>
<RevisionTitle>{revisionDate}</RevisionTitle>
{Object.keys(items).map(
Copy link
Owner

Choose a reason for hiding this comment

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

Use lodash.toPairs here, instead of iterating over the keys, so that you have easy access to both the keys and values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lodash.toPairs throws Object(...) is not a function

Copy link
Owner

Choose a reason for hiding this comment

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

You will need to look at the documentation

modules/gob-web/modules/course/revisions.js Outdated Show resolved Hide resolved
@hawkrives
Copy link
Owner

Lodash's toEntries plays nicer with flow. Please use it.

@drewvolz drewvolz changed the title Revisions list on course modal [WIP] Revisions list on course modal Oct 8, 2018
@@ -30,17 +31,17 @@ export class Revisions extends React.Component<Props> {
)

return (
<ul key={index}>
<BulletedList key={index}>
<RevisionTitle>{revisionDate}</RevisionTitle>
Copy link
Owner

@hawkrives hawkrives Oct 8, 2018

Choose a reason for hiding this comment

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

This needs to be in a ListItem as well, if you go this route. You should have

  • Date
    • enrolled: 31
    • max: 32

Copy link
Owner

Choose a reason for hiding this comment

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

This still applies

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. I was updating the branch to match master, so I haven't attempted this yet. It looks like we'll need to build in indented list-item support to the list module.

@hawkrives
Copy link
Owner

I'd like something like this, maybe

## October 2, 2017
Key      | Before | After
-------- | ------ | -----
enrolled | 31     | 32
max      | 15     | 35

## October 1, 2017
Key      | Before | After
-------- | ------ | -----
enrolled | 30     | 31
max      | 30     | 15

I'd like to be able to see what changed on each day, without having to mentally cross-reference the days.

I'm not tied to the table, but something like that to present all the information compactly.

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