-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Date range selector for stats #4302
Date range selector for stats #4302
Conversation
Generated by 🚫 Danger |
Ooh! Very cool! Screenshots? Thanks, Cess! |
|
fe538ed
to
69b6b9f
Compare
Hmm, maybe there's a not-closed HTML tag? A fix would be great!
…On Fri, Dec 14, 2018 at 4:10 PM Cess ***@***.***> wrote:
[image: range2]
<https://user-images.githubusercontent.com/17081074/50027442-57d22a00-fffd-11e8-9c44-3c3032f50f71.png>
Here it is @jywarren <https://github.com/jywarren>.
Also I realized the footer in this page looks weird.
https://publiclab.org/stats/range/ not same as https://publiclab.org/stats.
Should it be like that or should I try and fix it? Thanks
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4302 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJwsFkIcSa6ZFthtHRF5Y9mdCPPheks5u5BO0gaJpZM4ZUKUy>
.
|
Actually, found the bug on the footer, it was a missing '/' in a closing table tag 😄 It's fixed |
This is great. I'm thinking also we might have a way to auto-select some useful date ranges, like "Past 7 days" or "Past month" and such. See how in this date chooser, those are offered at the top? We could start with just a few as buttons: But that can be in follow-up to this PR. Is this ready to merge? So great!!!!! 👍 |
I have it default selected |
Hi, @cesswairimu ! CodeClimate is warning us about something here; would you mind clicking the icon above the notice to learn about what it's objecting to? It may be a security issue, but let's just be sure: https://codeclimate.com/github/publiclab/plots2/pull/4302 -- if it doesn't seem critical, we could in theory ignore it. Thanks! Otherwise this looks lovely. Maybe we could get the "Today" to align a bit better in the blue button, and add a 2-3px padding inside that button? Let me take a look at the code driving it. Once we resolve this CodeClimate issue we should be good to go. |
app/views/stats/range.html.erb
Outdated
<p> | ||
<button data-placement="bottom" data-toggle="popover" data-container="body" data-placement="bottom" type="button" data-html="true" class="btn btn-primary"> | ||
<span><i class="fa fa-calendar fa-2x"> </i> Today </span> </button> | ||
</button> Compared to <%= Date.today.to_formatted_s(:long) %> |
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.
Ah, here maybe there is a 2nd redundant button
element causing trouble? Also, is the span
totally necessary? It might be causing some layout issues too... just not totally sure it's required. Thanks!
app/views/stats/range.html.erb
Outdated
<h4><b> Select a range below to view stats within the range </b></h4> | ||
<%= form_tag request.url, method: 'get' do %> | ||
<div class="row"> | ||
<div class="col-md-8" style="background:#F5F6F8;"> |
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.
Here you could use the well
css class from Bootstrap instead of a background css style. Take a look!
<h5> Date Range </h5> | ||
<%= select_tag :options, options_for_select(["Week", "Month","Year"]), prompt: "View stats in the past", class: " form-control input-lg", onchange: "this.form.submit();" %> | ||
</div> | ||
</div> |
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 know it's a bit overly detailed to ask, but if you could keep all the indentations really consistent it does pay off in the long run across our whole project. Thanks, @cesswairimu !!!
5fbc450
to
a96fe2b
Compare
It looks good but perhaps we could use the bootstrap label styles for the
form input labels? What do you think?
Thanks!
…On Wed, Dec 26, 2018, 10:35 AM Cess ***@***.*** wrote:
[image: refac]
<https://user-images.githubusercontent.com/17081074/50449727-211fdc00-093a-11e9-9fc1-3d844252093d.png>
removed the today from the calendar icon and moved the range details to
the calendar icon. Made more sense to me this way because the range changes
and would not be compared with today. How does this look?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4302 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJzzxQtViEZI2ChKUTroznb4SbNBIks5u85c0gaJpZM4ZUKUy>
.
|
Cool on it. Thanks |
a96fe2b
to
c4b701a
Compare
Looks fantastic. Merging this now!!! |
* date-range selector for range * fix a missing '/' in table * style calendar selector
Fixes #4297
rake test
@publiclab/reviewers
for help, in a comment below