Remove bibliotron-article
#626
Conversation
🦋 Changeset detectedLatest commit: 61851d0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
|
Size Change: -120 B (0%) Total Size: 825 kB
ℹ️ View Unchanged
|
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (c9ae161) and published it to npm. You Example: yarn add @khanacademy/perseus@PR626 |
| @@ -469,15 +469,6 @@ | |||
| text-align: center; | |||
| } | |||
|
|
|||
| .bibliotron-article.framework-perseus.perseus-article { | |||
There was a problem hiding this comment.
Given this touches the perseus editor less file, we'll want to double-check how the article editor looks on a ZND
| @@ -34,6 +34,9 @@ class ArticleRenderer extends React.Component<any, any> { | |||
| ]).isRequired, | |||
|
|
|||
| // Whether to use the new Bibliotron styles for articles | |||
| /** | |||
| * @deprecated Does nothing | |||
There was a problem hiding this comment.
Is it possible to cut this out entirely? Or does that cause too much churn?
There was a problem hiding this comment.
@jeremywiebe I am just not familiar enough with Perseus to know how to remove this without causing errors to throw. I assumed it would be called by the consumer somehow?
There was a problem hiding this comment.
I figured that would be a breaking change to the external API. Please let me know if I'm mistaklen.
There was a problem hiding this comment.
It would be. But if we're doing this work, it'd be good to clean up this (now dead) flag. In webapp and mobile, we always set the value to true so it'd just be a matter of deleting those props.
There was a problem hiding this comment.
Ok-- well, we'd have to do that in a follow-up task, and we can't remove it here until it's been removed from those two places, right?
There was a problem hiding this comment.
Or I guess it could be part of a deploy but that's leaving it down to whomever cuts the next release to know to do that or break articles everywhere
There was a problem hiding this comment.
I think making tasks to clean it up for webapp and mobile first, which blocks the task to remove it here, would be a better move than just doing it now, right?
There was a problem hiding this comment.
I'd rather it all happen at once. We have a history of making cleanup tasks and leaving them undone. If you land this change, you can also cut a Perseus release and then follow up by updating webapp with the new version and the accompanying changes. I'm ok if Mobile is left for now as it's a super tiny change and will be very easy to adjust when we upgrade Perseus there.
| @@ -87,12 +45,13 @@ | |||
| .body-text; | |||
| margin: 0 auto; | |||
| max-width: @articleMaxWidth; | |||
| } | |||
| .perseus-renderer > .paragraph .paragraph { | |||
There was a problem hiding this comment.
So we don't need to specify only .paragraph's inside .perseus-renderer's anymore?
| <div id={"perseus-widget-passage-container" + joinedTitle}> | ||
| <div className="perseus-widget-passage-container"> |
There was a problem hiding this comment.
Sweet! Glad we could revert back to using class
There was a problem hiding this comment.
Yeah, probably less churn on the browser
nedredmond
force-pushed
the
ned/LC-1082-rm-bibliotron-article
branch
from
74fea6b
to
61851d0
Compare
Removes
bibliotron-articleclass, which also fixes line number alignment issue on article passagesLC-1082 (subtask of LC-596)
LC-1072