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

ProgressTableContainer - progress table refactor #38909

Merged
merged 1 commit into from
Feb 5, 2021

Conversation

maureensturgeon
Copy link
Contributor

@maureensturgeon maureensturgeon commented Feb 4, 2021

Dependent on #38899

This is a new component which will replace the VirtualizedSummaryView and VirtualizedDetailView a combined effort between @cforkish and @maureensturgeon, and was extracted from the full refactor work in #38309.

Unit tests were PR'd previously: #38625

Content view will render the following:
Summary view:
Screen Shot 2021-02-04 at 11 23 38 AM

Detail view:
Screen Shot 2021-02-04 at 11 24 00 AM

You can refer to this tech spec to see how this component fits into the full table architecture.

Links

Testing story

Reviewer Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@maureensturgeon maureensturgeon marked this pull request as ready for review February 4, 2021 19:28
@maureensturgeon maureensturgeon requested review from a team and jamescodeorg February 4, 2021 19:39
Copy link
Contributor

@jamescodeorg jamescodeorg left a comment

Choose a reason for hiding this comment

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

Two high-level questions, mostly for my understanding:

  1. Why do we have the student list and content view as two separate components (as opposed to the students just being another column in a combined table)?
  2. Why do we pass the legend through this component as a child prop (instead of just having ProgressTableSummaryView and ProgressTableDetailedView render it directly)?

}
};

class ProgressTableContainer extends React.Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

(minor) I think a comment here about the purpose of this component would be helpful. Is it mainly to synchronize the scrolling between the student list and the bubbles?

Copy link
Contributor

@cforkish cforkish Feb 5, 2021

Choose a reason for hiding this comment

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

i want to rename this component. the abstraction should be "i am a section progress table, just tell me how to render lesson cells" -- which i don't think is implicit with this name. the underlying implementation of needing to synchronize two tables shouldn't be relevant to anyone wanting to render a section progress table. unless i hear any better suggestions, i'm going to update this to SectionProgressTable. i meant to add a comment to each of these components before opening PRs (similar to what i wrote up in the tech spec), but forgot to do so before handing off to Maureen. i will do so while renaming.

@maureensturgeon
Copy link
Contributor Author

Two high-level questions, mostly for my understanding:

  1. Why do we have the student list and content view as two separate components (as opposed to the students just being another column in a combined table)?
  2. Why do we pass the legend through this component as a child prop (instead of just having ProgressTableSummaryView and ProgressTableDetailedView render it directly)?

I might defer to @cforkish on these

@cforkish
Copy link
Contributor

cforkish commented Feb 5, 2021

1. Why do we have the student list and content view as two separate components (as opposed to the students just being another column in a combined table)?

student list is its own component because in reactabular, sticky columns are implemented as a separate table with synchronized vertical scroll. the appropriate mental model here is to think of ProgressTableContainer as the "combined table" you mention. in fact i've been more and more thinking we should rename it to SectionProgressTable to make that more implicit... i just didn't want to have names like SectionProgressTableContentView. i specifically didn't use ProgressTable because we already have a component with that name, but now that i think about it, one might assume all these ProgressTableXXX components are related to that, so maybe SectionProgressTableXXX would actually be better anyway, despite being such a mouthful.

anyway, the student list is a component responsible for rendering the fixed column of the table, the content view renders the scrolling columns, and ProgressTableContainer represents the whole table.

2\. Why do we pass the legend through this component as a child prop (instead of just having ProgressTableSummaryView and ProgressTableDetailedView render it directly)?

ProgressTableContainer was intended to abstract as much common functionality as possible out of ProgressTableSummaryView and ProgressTableDetailView as possible. both tables include a legend, so it made sense to me to have the abstraction represent "section progress table with legend". however, @maureensturgeon also found this to be weird, so i guess this abstraction isn't as intuitive as i thought. i will update it.

Base automatically changed from maureen/LP-1719-progress-table-content-view to staging February 5, 2021 20:16
Copy link
Contributor

@cforkish cforkish left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@jamescodeorg jamescodeorg left a comment

Choose a reason for hiding this comment

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

I have another idea or two, but let's table (har har) them for another day.

@maureensturgeon maureensturgeon merged commit 9bc2931 into staging Feb 5, 2021
@maureensturgeon maureensturgeon deleted the maureen/LP-1683-progress-table-container branch February 5, 2021 22:49
@cforkish cforkish added the student progress PRs related to student progress label Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
student progress PRs related to student progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants