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

Staffhours/stats #373

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

vaibhavjayaraman
Copy link

Changes format of staffhours page, adds metabase graphs to mirror stats page

@abizer
Copy link
Member

abizer commented Mar 6, 2018

please include a screenshot and also separate the mirrors addition into a different PR

@@ -12,7 +12,7 @@
from ocfweb.component.lab_status import get_lab_status


@periodic(60, ttl=86400)
@periodic(60)
Copy link
Member

@chriskuehl chriskuehl Mar 6, 2018

Choose a reason for hiding this comment

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

why this change?

this was intentional so that when people screw up the file format (happens all the time) the home page will just have stale data (rather than error out)

the 60 means it still gets updated every 60 seconds when everything is happy

</div>
{% endfor %}
</div>

</script>
Copy link
Member

Choose a reason for hiding this comment

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

wat?

@@ -1,8 +1,20 @@
from django import template

from ocfweb.templatetags.lab_hours import lab_hours_holiday as holiday_hours
Copy link
Member

Choose a reason for hiding this comment

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

why renaming this function during import? it's better to avoid this except when necessary as it leads to confusion about what function it really is

@@ -6,6 +6,8 @@ $(document).ready(function() {
$('body').css('margin-bottom', height);
};

$('[data-toogle="tooltip"]').tooltip();
Copy link
Member

Choose a reason for hiding this comment

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

toogle?

@@ -0,0 +1,13 @@
.mirrors-pie-chart {
Copy link
Member

Choose a reason for hiding this comment

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

this needs a .page-mirrors or whatever wrapping the entire CSS file, see the other page-specific CSS files for examples

</div>
<div class= "mirrors-mirror-usage">
<iframe src="https://metabase.ocf.berkeley.edu/public/question/23f5e403-315c-432e-ae9f-3db0970517f7"
frameborder="0"width="100%"height="100%"allowtransparency></iframe>
Copy link
Member

Choose a reason for hiding this comment

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

spaces between html attributes

{% stats_navbar %}
<h2> Bandwidth usage per mirrored project </h2>
<div class="mirrors-pie-chart">
<iframe src="https://metabase.ocf.berkeley.edu/public/question/b8783817-1e98-4658-ae8a-2826dbeff4ab" frameborder="0" width="100%" height="100%"allowtransparen></iframe>
Copy link
Member

Choose a reason for hiding this comment

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

spaces between html attributes, fix weird spacing

<div class="col-md-6">
<h4> Bandwidth Usage Since {{ start_date }} </h4>
<br />
<br/>
Copy link
Member

Choose a reason for hiding this comment

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

why

<div class = "day">
<div class="daytitle">
<strong><big>{{staff_day.day}}</big></strong>
{%if staff_day.holiday != None %}
Copy link
Member

Choose a reason for hiding this comment

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

{%if staff_day.holiday is not none %}

Copy link
Member

Choose a reason for hiding this comment

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

@vaibhavjayaraman To explain this, is checks object identity eg if they are the same thing, == checks equality. Since None is a singleton (there can only be one), is/is not is faster and easier to read.

<div class="daytitle">
<strong><big>{{staff_day.day}}</big></strong>
{%if staff_day.holiday != None %}
<span class="cancelled-text"> {{staff_day.holiday}} </span>
Copy link
Member

Choose a reason for hiding this comment

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

Why the extra spaces between the <span> and </span>?

Vaibhav Jayaraman added 5 commits April 12, 2018 18:21
hours for staff hours are above each staff member

staff-hours grouped by hour functionally working

added reason for why ason for why staff hours are cancelled to header of staff hours

removed print statementint statements and reason for cancellation is next to staff hours

got rid of borders in individual days in staff hours

<hr /> added around each hour in staff hours

changed hr tag

added comment

changed weekdays to days in the week

functionally working, need to delete last hr tag

got rid of hr tag

Fixed some formatting issues

readded gray out feature to staff hours hours

adds if holiday will cancel

jan 17

some yaml changes made

changed some of staff_hours.html to adhere to ocflib

Altering home.py for staff_hours column in it

tuesday

made changes to staff_hours.html font size for different days

worked on staff_hours.html and staff_hours css

changed home.html staff hours and related css

got rid of cancelled staff hours from each day

added metabase graphs to mirrors.html

finished adding staff bios

width for staff-faces set to 175px

changed staff-hour spacing to 165px and can handle multiple lines of staff per hour in staff hours
{% if staff_hour.cancelled or staff_hour.day == today and lab_status.force_lab_closed %}
<span class="cancelled-text">cancelled this week</span>
{% for staff_day in staff_hours %}
<div class = "day">
Copy link
Member

Choose a reason for hiding this comment

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

Let's please not have spaces around the equal sign

@@ -108,8 +108,11 @@ <h3>OCF Computer Lab</h3>
</ul>

<div class="ocf-staffhours">
<span class="title">
Copy link
Member

Choose a reason for hiding this comment

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

This should be indented

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, indentation on the whole <span> element would be good.

<div class="hour {% if staff_hour.cancelled %} cancelled {% endif %}">
<div class="title">
<strong>{{staff_hour.time}}</strong>
{% if staff_hour.cancelled or lab_status.force_lab_closed and staff_day == today%}
Copy link
Member

Choose a reason for hiding this comment

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

nit: should have space before %}

<div class="title">
<strong>{{staff_hour.time}}</strong>
{% if staff_hour.cancelled or lab_status.force_lab_closed and staff_day == today%}
<span class="cancelled-text"> cancelled this week</span>
Copy link
Member

Choose a reason for hiding this comment

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

Is the space before "cancelled" necessary?

margin-bottom: 20px;

h4,
h5 {
Copy link
Member

Choose a reason for hiding this comment

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

Please fix this indentation

{% block content %}
<div class="ocf-content-block">
{% stats_navbar %}
Copy link
Member

Choose a reason for hiding this comment

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

Why has this been de-indented

Copy link
Member

@jvperrin jvperrin left a comment

Choose a reason for hiding this comment

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

I've got a few comments but not too much so far, could you give screenshots of the different pages that are modified in this PR so we can see what they look like?

@@ -108,8 +108,11 @@ <h3>OCF Computer Lab</h3>
</ul>

<div class="ocf-staffhours">
<span class="title">
Copy link
Member

Choose a reason for hiding this comment

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

Agreed, indentation on the whole <span> element would be good.

<div class="row">
{% stats_navbar %}
<h2> Bandwidth usage per mirrored project </h2>
<div class="row">
Copy link
Member

Choose a reason for hiding this comment

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

Yeah I don't think any changes are needed for this page from the looks of things

padding: 10px;
margin: 0;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think the addition of this file probably belongs in a separate pull request, since these aren't used at all yet.

Copy link
Author

Choose a reason for hiding this comment

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

Changes to staff hours page and staff hours section of home page.
screenshot from 2018-04-30 15-46-09

screenshot from 2018-04-30 15-41-52
screenshot from 2018-04-30 15-42-06
screenshot from 2018-04-30 15-42-15
screenshot from 2018-04-30 15-42-28

@kkuehlz
Copy link
Member

kkuehlz commented Jun 25, 2018

@vaibhavjayaraman hello. friendly reminder to revisit this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants