-
Notifications
You must be signed in to change notification settings - Fork 351
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
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 |
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.
Niiiice.. Glad we're starting down this road.
@@ -469,15 +469,6 @@ | |||
text-align: center; | |||
} | |||
|
|||
.bibliotron-article.framework-perseus.perseus-article { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to cut this out entirely? Or does that cause too much churn?
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we don't need to specify only .paragraph
's inside .perseus-renderer
's anymore?
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.
<div id={"perseus-widget-passage-container" + joinedTitle}> | ||
<div className="perseus-widget-passage-container"> |
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.
Sweet! Glad we could revert back to using class
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.
Yeah, probably less churn on the browser
74fea6b
to
61851d0
Compare
Removes
bibliotron-article
class, which also fixes line number alignment issue on article passagesLC-1082 (subtask of LC-596)
LC-1072