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

Add relative date feature #908

Merged
merged 3 commits into from
Apr 8, 2020
Merged

Add relative date feature #908

merged 3 commits into from
Apr 8, 2020

Conversation

alyip98
Copy link
Contributor

@alyip98 alyip98 commented Jun 27, 2019

What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [x] New feature

Addresses #901

What is the rationale for this request?
Adds the ability to insert relative dates

What changes did you make? (Give an overview)
Add a new markdown-it plugin that looks for @date Week#day@ tokens and replaces them with
dates relative to a specified base date.

Changed to a Nunjucks filter extension:
{{ baseDate | date(format, dayOffset) }}

Provide some example code that this change will affect:

Is there anything you'd like reviewers to focus on?
Is the proposed syntax suitable, or should we implement it as a tag e.g. <date>

Testing instructions:
Netlify preview

Proposed commit message: (wrap lines at 72 characters)
Add a new nunjucks filter to allow dynamic date calculation given
a base date and day offset and customizable formatting of the output.

@alyip98
Copy link
Contributor Author

alyip98 commented Jun 27, 2019

This is still an early WIP, but I want to get some opinions on where the base date should be defined, and whether the syntax is suitable/user friendly

@damithc
Copy link
Contributor

damithc commented Jun 27, 2019

Note that the date may need to be embedded in other text. e.g., Remember to submit by the deadline (Jan 12, 2019) in the right format ...

It would be nice if a site can support multiple timelines too. The current approach seems to be limited to one timeline per site (because the reference date is fixed to the base variable)

*/
function parseInput(match) {
// Parse week number
if (isNaN(match[1])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this ever be true, since the regex only matches digits? Also can change the regex for this to \d\d? since week should only be 2 digits.


// Parse day
let day = match[2].toLowerCase();
if (isNaN(day)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should check the length instead, 3 character / 1 character. \w{3} also matches 3 digits. which may not be intended.

Copy link
Contributor

Choose a reason for hiding this comment

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

If day is 001 it'll still be correct and interpreted as 1. Is this intended? Probably not a big deal either way

@openorclose openorclose marked this pull request as ready for review August 2, 2019 04:25
@damithc
Copy link
Contributor

damithc commented Feb 20, 2020

Shall we revive this PR @alyip98

@alyip98 alyip98 changed the title [WIP] Add relative date feature Add relative date feature Feb 21, 2020
@alyip98 alyip98 force-pushed the 901-rel-dates branch 4 times, most recently from 79e7c22 to db1e095 Compare February 23, 2020 08:57
Copy link
Contributor

@openorclose openorclose left a comment

Choose a reason for hiding this comment

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

Nice error handling. What about printing the error message and the file location too, in case authors publish without checking?


// Parse day
let day = match[2].toLowerCase();
if (isNaN(day)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If day is 001 it'll still be correct and interpreted as 1. Is this intended? Probably not a big deal either way

return formatError(DAY_INVALID, raw);
}
} else {
day = parseInt(day);
Copy link
Contributor

Choose a reason for hiding this comment

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

Still recommended to have the base (and i think we do include the base in other code)

Suggested change
day = parseInt(day);
day = parseInt(day, 10);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It defaults to 10, and in this case it's impossible to have 0x in the string so it seems unnecessary

}

let formatPattern = match[4] || DEFAULT_FORMAT;
formatPattern = formatPattern === '' ? DEFAULT_FORMAT : formatPattern;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be able to reduce to

Suggested change
formatPattern = formatPattern === '' ? DEFAULT_FORMAT : formatPattern;
const formatPattern = match[4] === '' ? DEFAULT_FORMAT : formatPattern;

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

match[4] could be empty string or undefined

@alyip98 alyip98 force-pushed the 901-rel-dates branch 2 times, most recently from 5442e5e to ec25bba Compare February 26, 2020 08:44
@alyip98
Copy link
Contributor Author

alyip98 commented Feb 26, 2020

Ready for review!


The semester starts on @date:{{ sem_start }} W1#1@

Lab 1 is due on @date:{{ sem_start }} W3#5@
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to find a way to escape the mustache syntax.

@alyip98
Copy link
Contributor Author

alyip98 commented Mar 12, 2020

Updated the PR with details regarding the new implemenation, i.e. syntax is now {{ baseDate | date(format, dayOffset) }} and it leverages nunjucks rather than adding a new "tag" symbol.

Please let me know if there are any concerns or improvements

Copy link
Member

@yamgent yamgent left a comment

Choose a reason for hiding this comment

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

This new syntax is definitely much cleaner, nice job. 👍

test/functional/test_site/testDates.md Outdated Show resolved Hide resolved
docs/userGuide/syntax/dates.mbdf Outdated Show resolved Hide resolved
docs/userGuide/syntax/dates.mbdf Show resolved Hide resolved
docs/userGuide/syntax/dates.mbdf Show resolved Hide resolved
return Moment(base).add(days, 'days');
}

function filter(baseDate, format = DEFAULT_FORMAT, day = 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to allow the user to set a DEFAULT_FORMAT? Maybe somewhere in site.json.

Then we can change the arguments to filter to filter(baseDate, day = 0, format = DEFAULT_FORMAT).

This is assuming that the most frequent use case would be the author having only 1 format they want to use, so being able to do {{ base | date(7*2 + 1) }} and avoid typing the format all the time might be easier syntax.

Copy link
Contributor Author

@alyip98 alyip98 Mar 16, 2020

Choose a reason for hiding this comment

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

Fair point, I suppose if there really was a need we could also split the filter into 2 separate filters in a future PR, like {{ baseDate | addDays(7 * 2 + 1) | formatDate(format) }}.

Configuring a custom DEFAULT_FORMAT isn't too hard but I'm not sure if it will be very useful, since an extra argument isn't a hassle. An alternative is to define a nunjucks macro (customFormat defined in variables.md)

{% macro customDate(baseDate, days) %}
{{ baseDate | date(customFormat, days) }}
{% endmacro %}

Usage: {{ customDate(baseDate, 5) }}

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we still allow this, but without the configuration? :o
( swapping the day and format in format = DEFAULT_FORMAT, day = 0) { )

@damithc
Copy link
Contributor

damithc commented Mar 16, 2020

image

Copy link
Contributor

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

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

LGTM in general 👍; The require('nunjucks').configure() could be restructured to apply to all three files.
Just some small nits otherwise:

docs/userGuide/syntax/dates.mbdf Outdated Show resolved Hide resolved
docs/userGuide/syntax/dates.mbdf Outdated Show resolved Hide resolved
return Moment(base).add(days, 'days');
}

function filter(baseDate, format = DEFAULT_FORMAT, day = 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we still allow this, but without the configuration? :o
( swapping the day and format in format = DEFAULT_FORMAT, day = 0) { )

src/lib/markbind/src/parser.js Outdated Show resolved Hide resolved
test/unit/parser.test.js Outdated Show resolved Hide resolved
docs/userGuide/syntax/dates.mbdf Outdated Show resolved Hide resolved
test/functional/test_site/testDates.md Show resolved Hide resolved
Copy link
Contributor

@openorclose openorclose left a comment

Choose a reason for hiding this comment

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

The {{ base | date(format, 7*2 + 1) }} method should be quite clear for now.

My concerns about the format param not being used too often may be unfounded after users test it out, and we can create another issue to swap the params if the format parameter is left mostly to some default.

src/Page.js Outdated Show resolved Hide resolved
Copy link
Contributor

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

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

Just one last nit, could resolve the conflicts as well!

test/functional/test_site/testDates.md Show resolved Hide resolved
src/lib/markbind/src/parser.js Outdated Show resolved Hide resolved
docs/userGuide/syntax/dates.mbdf Outdated Show resolved Hide resolved
src/lib/markbind/src/utils/nunjuckUtils.js Outdated Show resolved Hide resolved
src/lib/markbind/src/utils/nunjuckUtils.js Outdated Show resolved Hide resolved
src/Site.js Outdated Show resolved Hide resolved
@alyip98 alyip98 force-pushed the 901-rel-dates branch 2 times, most recently from 9f5eb2d to b23fe59 Compare April 4, 2020 12:45
@alyip98 alyip98 force-pushed the 901-rel-dates branch 2 times, most recently from 5145603 to 21e68fa Compare April 4, 2020 13:12

{{ base2 | date(format2, 10) }} should be Fri 11/01

{{ formatted_date }} should be Tue 1 Jan
Copy link
Contributor

Choose a reason for hiding this comment

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

Fri 11 Jan

LGTM otherwise, nice work on the njutil refactor 👍

@ang-zeyu ang-zeyu added this to the v2.13.2 milestone Apr 4, 2020

The baseDate follows the format: `YYYY-MM-DD`

It can be supplied using [page variables](../reusingContents.html#variables) for convenience.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can elaborate on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The format portion or the page variables portion?

Copy link
Contributor

Choose a reason for hiding this comment

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

page variable portion. May be give an example?

Copy link
Contributor

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

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

Noticed a few more doc nits, could polish it up here or in a separate PR

  • could standardise with the level of subheadings used elsewhere in the page h2 -> h4 -> h5
  • #### Custom formatting seems to be a repeat of ### Advanced formatting
  • Could combine page variables into using variables

@alyip98
Copy link
Contributor Author

alyip98 commented Apr 5, 2020

Noticed a few more doc nits, could polish it up here or in a separate PR

  • could standardise with the level of subheadings used elsewhere in the page h2 -> h4 -> h5
  • #### Custom formatting seems to be a repeat of ### Advanced formatting
  • Could combine page variables into using variables

Let's do this in a separate PR, it can be a good first issue

@ang-zeyu
Copy link
Contributor

ang-zeyu commented Apr 5, 2020

Let's do this in a separate PR, it can be a good first issue

👍, seems good to be merged in that case. Do open up an issue!

@alyip98 alyip98 merged commit b20f421 into MarkBind:master Apr 8, 2020
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Apr 9, 2020
* 'master' of https://github.com/MarkBind/markbind: (41 commits)
  Document adding new site content in DG (MarkBind#1153)
  Add relative date feature (MarkBind#908)
  Use <br> to separate lines of code block (MarkBind#1176)
  Parse popovers for footnotes (MarkBind#1155)
  Resolve comments
  Expand test extensions and fix whitespace checks (MarkBind#1156)
  Address comments
  Upgrade js-beautify and provide option to turn it off (MarkBind#1116)
  Normalize inline puml line ending before hashing (MarkBind#1174)
  Update tests (MarkBind#1178)
  Remove fixed bugs from test\functional\test_site\bugs\index.md` (MarkBind#1148)
  Fix bug in Search for UG and DG (MarkBind#1147)
  Add inline puml support (MarkBind#1100)
  Fix hrefs for headings with angular brackets (MarkBind#1089)
  Update tests for 2.13.1 (MarkBind#1169)
  2.13.1
  Update vue-strap version to v2.0.1-markbind.39
  Fix fontawsome icons don't show underlines to indicate modal/tooltip (MarkBind#1133)
  2.13.0
  Update test files
  ...
marvinchin pushed a commit that referenced this pull request Apr 10, 2020
Add a new nunjucks filter to allow dynamic date calculation given
a base date and day offset and customizable formatting of the output.
@damithc
Copy link
Contributor

damithc commented Apr 18, 2020

Seems to be working in production. Good work @alyip98 and reviewers. It will be very useful for the module websites next semester.

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.

6 participants