-
Notifications
You must be signed in to change notification settings - Fork 23
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
Highlight certain dates in the calendar datepicker #259
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #259 +/- ##
==========================================
- Coverage 38.84% 38.84% -<.01%
==========================================
Files 47 47
Lines 7420 7444 +24
==========================================
+ Hits 2882 2891 +9
- Misses 4538 4553 +15
Continue to review full report at Codecov.
|
Regarding checking if certain dates URLs are available to highlight - it's probably better for this to be done server side using PHP and then passing the variable to the JavaScript. I'll have a loot into making this change. |
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.
@mattpitkin, just a couple of minor things to change, otherwise this looks awesome, thanks for putting it together. I do think this will be useful, especially for people using GWSumm outside of the production summary pages, which I'm on a mission to put more effort into supporting.
A couple of general thoughts:
- Can you post a working example of this new feature?
- Can you also update the (brand-new) unit test for
gwsumm.html.bootstrap.calendar
? - Since you're starting to become a regular contributor (which is awesome), in the future it might be worthwhile to post feature requests like this one as issue tickets so that we can coordinate your contributions with the release cycle.
@mattpitkin, we currently don't use PHP on the summary pages, so JavaScript is actually the preferred method of implementation, at least for now. I think once we have several months of O3 under our belt we can return to the question of whether PHP would be a worthwhile thing to support. |
@alurban just saw your comment about PHP, although in the latest commit I've actually added a PHP file that lists the directories that exist. This is a lot quicker than my previous JavaScript-only alternative. I've not checked it works fully yet, but let me know what you think. |
@mattpitkin, I think that in general we shouldn't move to support PHP. However, since I'm wholly ignorant of how PHP works, I'm going to request a review from @duncanmmacleod. |
@alurban after thinking a bit more, I can probably get rid of the PHP again and achieve the same thing, by getting |
… using gw_summary
@alurban I've now removed the need for the PHP, which is good 😄 I've also updated the unit test for calendar in I'll add a working example soon. |
For completeness I've just added an issue #262 that goes with this PR. |
An example ini file (
with the file
and
and given something that looks like: |
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.
@mattpitkin, this is looking really good, sorry for the delay getting back to you (we've been busy trying to upgrade the summary pages to conda). A few more comments are posted below, let me know what you think, and thanks for working on this!
gwsumm/html/bootstrap.py
Outdated
attributekwargs['highlight-dates'] = highlighteddates.replace('-', '') | ||
elif highlightavailable: | ||
# set location of dates file | ||
datefile = '{}/list-dirs.txt'.format( |
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.
@mattpitkin, see my comment to gw_summary
below. If we decide to keep this file rather than the raw list of dates, then this variable should actually be passed as a keyword argument with a default of None
, i.e.
def calendar(date, tag='a', class_='navbar-brand dropdown-toggle',
id_='calendar', dateformat=None, mode=None, datefile=None,
highlighteddates=None, highlightavailable=False):
That would make this a lot more robust as it would enforce that the file expected here be the same as the one defined in gw_summary
. Any calls to gwsumm.html.bootstrap.calendar
would have to be updated along with this function's docstring and unit tests.
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've made changes to pass a datefile
argument as suggested. In gw_summary
I get it to add the file name to the config
variable.
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.
@mattpitkin, a couple more comments and one request: after you make these changes, can you post a link to a simple working example? Otherwise, these changes look great.
bin/gw_summary
Outdated
datedirs = sorted([thisdir for thisdir in os.listdir(dirpath) | ||
if os.path.isdir(os.path.join(dirpath, thisdir))]) | ||
dirfile = os.path.join(dirpath, 'list-dirs.txt') | ||
config.set('calendar', 'date-file', dirfile) |
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.
@mattpitkin, my one concern here is that users won't be able to pass a date-file
in the INI configuration because it will be overwritten by the call to config.set
. I think you can work around this pretty easily though, by changing from set
to get
:
config.set('calendar', 'date-file', dirfile) | |
config.get('calendar', 'date-file', dirfile) |
I haven't tested this though, so you'll probably want to do that.
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.
In 224cbef I've made a change to check if the user is passing a date-file themselves.
Co-Authored-By: mattpitkin <[email protected]>
@mattpitkin Sorry that this PR has been left to linger for so long. I am unsure what the status of it was before @alurban left the collaboration to work in the green fields of industry. It appears that we won't be able to merge it since some of this PR is impacted by #289. However, maybe it can still be deployed but it will take a PR first over on gwdetchar/bootstrap and then possibly some changes in gwsumm. How do you want to move forward with this? We could close the PR so that you can split up the PR into the gwdetchar PR and a new gwsumm PR. What do you think? Thanks, and sorry again for the way-to-long delay |
@eagoetz no problem, I'd completely forgotten about this. I'll have a look and see how easy this is to split into two new PRs. If it's not too much work I'll do that, but otherwise we'll just have to close this. |
I'm using
gw_summary
to show summary pages that will only be produced on certain dates. This PR allows you to specify in the configuration file either a selection of specific dates to highlight, or whether the page will check URLs for dates that exist (non-highlighted dates will be disabled).In both methods I have it so that the requested dates are highlighted and other dates are not enabled. The latter method, which checks the URLs can be a bit slow in rendering the page, as it has to check the URLs synchronously before rendering.
To select the former option you would add e.g. (the dates can be with or without the hyphens)
and for the latter option you would add
If you have the
highlighted-dates
option, then that is favoured over thehighlight-available
option. If you have neither of these then things should render as normal.Currently, the options in the
calendar
section are read in in thefrom_ini
class method of theBaseTab
, as I couldn't find a better way to do it. I'd tried adding afrom_ini
method to theIntervalTab
in which to read in the values, but couldn't get that to work.Does this patch look ok/useful?