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

Scheduler scroll fix #3280

Merged
merged 2 commits into from
Mar 1, 2017
Merged

Conversation

SaptakS
Copy link
Contributor

@SaptakS SaptakS commented Mar 1, 2017

Fixes #3257

Works Done:

  • Show only rooms with sessions
  • Show time from first session to end of last session

See the screenshots below

screenshot from 2017-03-01 21-23-03
screenshot from 2017-03-01 21-23-08

@mariobehling @niranjan94 pease review.

var dayIndex = _.indexOf(days, day);

if (/.+\/e\/.+\/schedule\//i.test(document.URL)) {
Copy link
Member

Choose a reason for hiding this comment

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

What is this test for ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is so that this changes take place only in the public page and not in the scheduler in event dashboard. Because in event dashboard we need the entire time rather than from start session to end session. Same for the room.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@niranjan94 niranjan94 Mar 1, 2017

Choose a reason for hiding this comment

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

@SaptakS use the isReadOnly() function. isReadOnly() returns a true if the scheduler is running in the public page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok cool.

@SaptakS SaptakS force-pushed the scheduler-scroll-fix branch 2 times, most recently from c62230c to 9c1f7b9 Compare March 1, 2017 16:08
@SaptakS
Copy link
Contributor Author

SaptakS commented Mar 1, 2017

@niranjan94 made the changes. Please review.

@@ -919,6 +957,12 @@ function loadMicrolocationsToTimeline(day) {
$("[data-toggle=tooltip]").tooltip("hide");

if (isReadOnly()) {
_.each($microlocations, function ($microlocation) {
$microlocation = $($microlocation);
if ($microlocation.find('.microlocation-inner').html().trim() === "") {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this, a more straightforward way would be to count the children.
$microlocation.find('.microlocation-inner').children().length;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would give the same result, right? Can you explain why this is a better approach?

Copy link
Member

Choose a reason for hiding this comment

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

The result would be the same ... In most cases ... But by chance (an edge case) due to some error in the template or js code, a non-whitespace character ends up inside the div. Then just trimming and equating it to empty space would fail.

And it's always better to look at the DOM as an object instead of a string.

@@ -849,6 +848,10 @@ function loadDateButtons() {
*/
function loadMicrolocationsToTimeline(day) {

$('table.timeline-table').show();
Copy link
Member

Choose a reason for hiding this comment

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

@@ -849,6 +848,10 @@ function loadDateButtons() {
*/
function loadMicrolocationsToTimeline(day) {

$('table.timeline-table').show();
$('#no-session-message').hide();
Copy link
Member

Choose a reason for hiding this comment

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

This too

});

if (max_hours === 0) {
$('table.timeline-table').hide();
Copy link
Member

Choose a reason for hiding this comment

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

These too

@codecov-io
Copy link

codecov-io commented Mar 1, 2017

Codecov Report

Merging #3280 into development will decrease coverage by -0.11%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           development    #3280      +/-   ##
===============================================
- Coverage        75.67%   75.57%   -0.11%     
===============================================
  Files              226      226              
  Lines            14899    14899              
===============================================
- Hits             11275    11260      -15     
- Misses            3624     3639      +15
Impacted Files Coverage Δ
app/helpers/deployment/heroku.py 59.09% <0%> (-27.28%)
app/views/super_admin/super_admin.py 71.42% <0%> (-12.5%)
app/helpers/helpers.py 48.98% <0%> (-0.68%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86e335d...51e190c. Read the comment docs.

@SaptakS SaptakS merged commit 57f883f into fossasia:development Mar 1, 2017
@SaptakS SaptakS deleted the scheduler-scroll-fix branch March 1, 2017 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants