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

ProgressTableLessonNumber #38215

Merged
merged 3 commits into from Jan 23, 2021
Merged

Conversation

cforkish
Copy link
Contributor

@cforkish cforkish commented Dec 11, 2020

this PR represents a combined effort between @cforkish and @maureensturgeon, and was extracted from the full refactor work in #38309.

this is a refactored version of SectionProgressLessonNumberCell for use in the refactored progress tables. the primary difference in this version is to render the tooltip directly in this component, which prevents us from having to re-render all tooltips when the table is scrolled.

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

Screen Shot 2021-01-22 at 10 46 33 AM

@cforkish cforkish force-pushed the cforkish/LP-1673-ProgressTableLevelIcon branch from 5c3c4bb to 67d8abf Compare December 11, 2020 23:08
@cforkish cforkish force-pushed the cforkish/LP-1674-ProgressTableLessonNumber branch from c8923bc to 3305ff5 Compare December 11, 2020 23:09
@cforkish cforkish force-pushed the cforkish/LP-1674-ProgressTableLessonNumber branch from 3305ff5 to f329166 Compare January 22, 2021 20:51
@cforkish cforkish changed the base branch from cforkish/LP-1673-ProgressTableLevelIcon to staging January 22, 2021 20:51
@cforkish cforkish requested review from a team and jamescodeorg January 22, 2021 20:55
@cforkish cforkish marked this pull request as ready for review January 22, 2021 20:56
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.

LGTM. I think the detail that you included in the PR description about why the tooltip is rendered in the component is important and should probably included in comments in the code somewhere?

it('renders the name in the tooltip', () => {
const wrapper = setUp();
const tooltipComponent = wrapper.find(ReactTooltip);
expect(tooltipComponent.contains(DEFAULT_PROPS.name)).to.equal(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be written as to.be.true.

@cforkish
Copy link
Contributor Author

@jamescodeorg personally i think that detail is important only to explain why that's in the diff. the previous implementation needed to include a comment (here) about rebuilding tooltips since they weren't rendered by the components that used them, but imo including the tooltip in the component that uses it is intuitive and needs no explanation. don't you think it would be a little weird to see a comment like this?

// Tooltip is rendered here because this is where it's used. if we rendered it somewhere else, 
// we would need to make sure to rebuild it when this component is scrolled.

happy to add it though if you think it's helpful.

@jamescodeorg
Copy link
Contributor

Heh, ok, point taken. I was thinking that it might be good to note that it's important to have the tooltip be rendered with the component for performance reasons but if it's the usual thing to do, then agree we don't need the comment. (Which makes me wonder, why did we do it the other way before?)

@cforkish cforkish added the student progress PRs related to student progress label Jan 22, 2021
@cforkish cforkish merged commit f0d84ba into staging Jan 23, 2021
@cforkish cforkish deleted the cforkish/LP-1674-ProgressTableLessonNumber branch January 23, 2021 01:11
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

2 participants