-
Notifications
You must be signed in to change notification settings - Fork 480
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
Update Course Description to allow for markdown and preview it on course edit page #36363
Conversation
@@ -21,6 +23,12 @@ const styles = { | |||
}, | |||
dropdown: { | |||
margin: '0 6px' | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this style is duplicated from #36346. I don't know what best practices are here - should we consider sharing it somehow? I assume we want consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to extract the Preview out into a component so it can be re-used. I'll do that in a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow up: #36367
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. in terms of best practices, within React code, I'm probably generalizing a bit too much here but I would say that good times to extract duplicate code are:
- when you can extract a whole react component (like this time, I think)
- when you can extract a constant which has some significance (maybe like
SCRIPT_OVERVIEW_WIDTH
) - when you can extract a "prop type" (see various shapes.js files. these would never include css)
otherwise, I wouldn't see a huge value in extracting two CSS values or blocks that happen to match between components, since it seems like it might create a tighter coupling between components that should be able to vary independently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ That makes sense. Here I was wondering if we would intentionally want to couple the styling between the different edit pages to keep it consistent. But extracting an entire component is better, yea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. DRY is inherently a bit harder to grapple with in front end code. Web sites which attempt to have consistent visual styles create a huge spectrum of similarity levels in the code (especially CSS) between different pages. React is nice in that it is opinionated about what should be encapsulated. Without a framework like React, if we just had a giant shared CSS file for example, and you were to write two editor pages which used the same set of styles, we'd be much more likely to extract a common CSS class and use it in 2 places, and then maybe make an effort to make individual CSS classes more reusable. but React's opinion is that Components should be the unit of encapsulation, so we focus on that instead.
to go on a bit... I do love the DRY principle but also feel like it would take a lifetime to get good at deciding when to apply it or not. It's tempting to apply whenever things look the same, and it's a good idea to apply it most of the time and always a good question to raise when code appears duplicated. OTOH if the two implementations should be allowed to diverge from each other then they should not share code even if it is identical. To endorse React's "opinion", I like the litmus test of whether a shared react component can be extracted, because if it cannot, that's a good (somewhat arbitrary, but good enough) indicator that they are different enough that they should not share implementation.
For the course and script edit pages in particular, I could imagine more components being extracted in this manner for anything shared that are not basic, independent html input types. I say "independent" because I think VisibleAndPilotExperiment
was a great example of something to extract because the fields are shared between the two editors and they also depend on each other in the same way in both cases.
(end rant :-P )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking of independent, in the other PR I suggested including both the preview and the textarea in the component, since they depend on each other. Does that seem reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, sounds great!
Does the same thing for Course that #36346 did for Unit.
Course Overview description shows markdown
Course Edit page previews descriptions
Links
Testing story
Reviewer Checklist: