-
Notifications
You must be signed in to change notification settings - Fork 37
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
Bug fixes after feedback from production push #80
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #80 +/- ##
========================================
Coverage 89.34% 89.34%
========================================
Files 65 65
Lines 3435 3435
========================================
Hits 3069 3069
Misses 366 366 Continue to review full report at Codecov.
|
@grozdanowski I just skimmed over. Will look over in more depth tomorrow. One thing we want to make sure is clear is when and how the custom yarn build needs to be done. What command do you run to build the assets we will commit to a release? I want to add that to the makefile. |
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.
Thanks @grozdanowski! Looks good to me.
I hope that we can revert the Appsembler preferred SASS variables change. It's unfair to the community that we do such change.
Instead of letting everyone maintain their REM_*
value, we should maintain our own.
@@ -19,7 +19,7 @@ class MauDetailsContent extends Component { | |||
<li key={index} className={styles['content-row']}> | |||
<span className={styles['period']}>{period.period}</span> | |||
<span className={styles['mau-count']}>{period.value}</span> | |||
<span className={cx({ 'difference': true, 'positive': (difference > 0), 'negative': (difference < 0)})}>{difference}</span> | |||
<span className={cx({ 'difference': true, 'positive': (difference > 0), 'negative': (difference < 0)})}>{(difference > 0) ? "+" : ""}{difference}</span> |
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.
Can we make this sign an emoji like 🔼 and 🔽
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.
@OmarIthawi we can, but I'd rather do it in FontAwesome than (ugly) emoji :) Let's keep this off for now - I don't think it's a priority.
@@ -39,6 +39,8 @@ class HeaderContentCourse extends Component { | |||
|
|||
render() { | |||
|
|||
const displayCourseHeaderGraph = false; |
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 please! The graph is broken in my opinion, thanks for removing it!
I should've reported the bug earlier, but for whatever reason I didn't.
@@ -24,7 +24,7 @@ class CoursesListItem extends Component { | |||
}); | |||
|
|||
return ( | |||
<div className={styles['course-list-item']} key={this.props.courseId}> | |||
<Link to={'/figures/course/' + this.props.courseId} className={styles['course-list-item']} key={this.props.courseId}> |
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.
👍
@@ -26,7 +26,7 @@ class App extends Component { | |||
<LoadingSpinner | |||
displaySpinner = {!(this.props.activeApiFetches === 0)} | |||
> | |||
<main className={styles['layout-root']}> | |||
<main id="main" className={styles['layout-root']}> |
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 sure @MHaton is happier now 😃
@@ -1,14 +1,14 @@ | |||
@import '~base/sass/base/functions'; | |||
|
|||
$rem-base-size: 16 !default; | |||
$rem-base-size: 10 !default; |
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.
Will this break when the community uses them on vanilla Open edX?
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.
@OmarIthawi If this does break for vanilla, I'll work out an interim solution. Not for now, but for later, is there a reason why an external css that defines the rem-base-size
? from which Figures uses. I mean, this is part of the cascade in CSS, right? That and we really want to move away from building the assets and including them in the repo, instead building the assets into the deployable python bundle we push to PyPI
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.
Thanks @johnbaldwin! I did use the wrong word here. I don't think it will "break break", and Matej explained that this is a temporary solution, which I think is OK.
I simply worried that the font will look very small on Open edX deployments to the point that forces community members to fork and recompile, which is anti-opensource.
we really want to move away from building the assets and including them in the repo, instead building the assets into the deployable python bundle we push to PyPI
Yes please! I really like that you're working on keeping PyPI as the no. 1 installation option.
If this does break for vanilla, I'll work out an interim solution. Not for now, but for later, is there a reason why an external css that defines the rem-base-size?
Yes, rem
is all about cascade/inheritance as far as I know. @grozdanowski knows better than I do, but I think it's more like saying font-size: 100%
. It's not just cascaded, it is also depends on what the parent font-size
value is.
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.
@johnbaldwin @OmarIthawi To clarify - there's no need to fork. One can pass on env variables and rebuild on the fly. This is also a convenient tool for other purposes (if someone wants to brand their Figures intance for themselves using font sizes or colours).
And yes - it is an issue in regards to cascading nature of CSS - rem
is defined by the first ever definition of font size in the rendered document.
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 looked over again. Don't have anything more to add. Thanks for reviewing @OmarIthawi ?
@grozdanowski Thanks for jamming this out this weekend! |
@grozdanowski I took the liberty to reword the title of this pull request 🙈 |
Thanks for reviewing this - merging. |
* limit course progress decimals number * fix how dates are displayed * add “+” or “-“ sign in front of difference figure on mau history * rename “Dashboard” to “Overview” * Display avg course progress as percentage * temporarily remove the graph that displays users progress through course * fix the color of user name link in learners list item * make entire course card in courses list clickable * add tooltips to mau graph * fix the history graph breakage * fix the learner statistics widget data display * add “main” ID to app main div so accessibility link works * set SASS variables defaults to Appsembler-preferred values * Rename cards on Dashboard, remove Courses enumeration card * remove a console.log * recompile assets
* Re-added the method filter update for django-filter 1.0.4 * Added 'start' and 'end' to the CourseOverview migration
These commits take care of the following bugs:
REM_BASE=16 yarn build
)