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 calendar block v1 #13772

Merged
merged 4 commits into from
Feb 14, 2019
Merged

Add calendar block v1 #13772

merged 4 commits into from
Feb 14, 2019

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Feb 8, 2019

Description

Closes #1462.

The first version of the calendar block.
The design tries to follow the mockup proposed but is not exactly equal as getting the desired result would need a change of markup while this version is a pure CSS solution.

If we decide to change the markup to get to the desired design I think we will not be able to use get_calendar as it is right now and the best option seems to be using a custom version of get_calendar creating a new function in core that abstracts get_calendar logic and allows both get_calendar and the version we use in this block to be created in a simple way.
We can also use current filters on get_calenger plus some regex replaces to get the markup in the desired state, but that is very hacky and probably not a long term solution.

How has this been tested?

Verify the calendar block works as expected in the editor and in the front end.
Verify that when we change the publish date month and/or year the calendar updates.

Screenshots

screenshot 2019-02-08 at 12 30 59

screenshot 2019-02-08 at 12 30 16

@youknowriad youknowriad added Needs Design Feedback Needs general design feedback. Needs Accessibility Feedback Need input from accessibility [Feature] Blocks Overall functionality of blocks [Feature] Widgets Screen The block-based screen that replaced widgets.php. labels Feb 8, 2019
@youknowriad youknowriad added this to the 5.1 (Gutenberg) milestone Feb 8, 2019
getServerSideAttributes( attributes, date ) {
return {
...attributes,
...this.getYearMonth( date ),
Copy link
Member

Choose a reason for hiding this comment

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

Why not set it on the server using WP functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

When the user changes the publish date, even if the post is not yet saved we want the user to see the calendar of that publish date so we need a way to pass that information to the server, that exactly what we are doing here.
The attributes month and year are never saved, they are just transient attributes used by the server side render mechanism.

Copy link
Member

Choose a reason for hiding this comment

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

I still think it isn't a good approach. Let me explain on the post from my personal website:
https://gziolo.pl/2017/11/07/starter-kit-and-reusable-scripts/
If I visit it as of today you will see that the calendar in the sidebar renders the same month as when this post was published:
screen shot 2019-02-14 at 11 29 11

it behaves differently for the pages as it simply renders the current month:
screen shot 2019-02-14 at 11 31 31

There are also archive pages which follow the URL path:

screen shot 2019-02-14 at 11 33 46

This should also be the case when I want to edit my post in Gutenberg. That's why I suggested that we build logic on PHP side which tries the following:

  • if it is a post type get the publish date from the post metadata
  • if it is a page or custom CPT get the current month

@melchoyce does it make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Thinking a bit more about it. I'm coming to the conclusion that it might be nice to explore the following UI improvement around month and date. What if we allow the author to pick what calendar would display, some options to consider:

  • inherit the post's date (this is what we have at the moment)
  • use the current date
  • let the author pick any date

/cc @mapk and @youknowriad for feedback

@gziolo gziolo requested review from melchoyce and mapk February 8, 2019 13:18
@jorgefilipecosta jorgefilipecosta force-pushed the add/calendar-block-ssr branch 2 times, most recently from 761c864 to e25812d Compare February 8, 2019 15:19
@melchoyce
Copy link
Contributor

Seeing a gap here where the header area doesn't totally extend the width of the block:

image

I can't click this arrow, and with it's position + no label, I don't think it's clear what it does:

image

It's more obvious in the date picker component because of proximity:

image

Where are the styles coming from? Are they the default calendar widget styles?

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

This is looking pretty cool! 😎

packages/block-library/src/calendar/index.php Outdated Show resolved Hide resolved
packages/block-library/src/calendar/index.php Outdated Show resolved Hide resolved
packages/block-library/src/calendar/index.php Outdated Show resolved Hide resolved
packages/block-library/src/calendar/index.php Show resolved Hide resolved
packages/block-library/src/calendar/index.js Outdated Show resolved Hide resolved
@mapk
Copy link
Contributor

mapk commented Feb 12, 2019

Can't wait to get this in!

I think Mel brought up an interesting correlation above between this block and the DatePicker component. Is it possible to get the Calendar to match the styles of the DatePicker?

cal-styles

Also noticed I can't interact with the arrow in the calendar to switch months, and when I tried to interact with it on the frontend, it took me to a different post completely.

@jorgefilipecosta
Copy link
Member Author

Hi @melchoyce, @noisysocks, and @mapk thank you for the reviews most of the points raized were addressed.

Where are the styles coming from? Are they the default calendar widget styles?

The styles are specific to this block they were created without changing the original markup using this image as referential:
image

Unfortunately, without changing the markup, I don't think it is possible to get the arrows in the right position.

I think this version is possible to achieve with the current markup:
calendar block unselected

As a reference the default calendar styles look like this:
screenshot 2019-02-12 at 15 12 43

I think Mel brought up an interesting correlation above between this block and the DatePicker component. Is it possible to get the Calendar to match the styles of the DatePicker?

Yes, it is possible to get the same look on the editor. In that case, probably it would be worth it to use the DatePicker component directly and create a JS UI instead of relying on ServerSide render, which is probably going to be useful anyway if we want to follow more advanced use cases.

I have some questions about the way calendar block should work on the frontend and what differences should exist when compared with the calendar widget.

Currently, the calendar widget renders the calendar of the month the current post was published, if the widget is an archives page it renders the calendar of that month, and if it is a neutral non-date specific page (homepage, taxonomy, author) it renders the calendar of the current month.
There is no interactivity on the widget frontend it just links to the archives of the previous/next month and the days.

The current calendar block implementation tries to follow the same approach.

The blocks show the calendar of the month associated with the post publish date. When we update the month/year of publish date, we can see on the editor the block also gets updated.

If we allow the user to scroll between months what is the practical effect of the scroll? Should we start showing the calendar of the month chosen by the user instead of relying on the publish date?

On the frontend of the website should the calendar also look like the DatePicker component or should different styles be used?

Regarding interactivity should the frontend also be interactive?
If yes we will face a new challenge, have core widgets that have JavaScript on the frontend.
Probably even if the styles are the same as DatePicker we will not be able to use DatePicker directly because that would make us need to include React on the frontend of the websites with the react-dates library which are huge.

@youknowriad
Copy link
Contributor

If yes we will face a new challenge, have core widgets that have JavaScript on the frontend.

How does the widget work, we should just do the same thing for the moment.

@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented Feb 12, 2019

How does the widget work, we should just do the same thing for the moment.

That's the current state we have exactly the same thing as the calendar widget on the frontend just with some styles changes (that can be reverted).

@melchoyce
Copy link
Contributor

Ah, yes. The styles came from my mockups. 🤦‍♂️

I think this version is possible to achieve with the current markup

If we can't change the markup, let's do either that, or the default calendar styles — they're actually not that bad. (@mapk, what do you think?)

Since you can't interact with the calendar months (I forgot about that part), can we do cursor: not-allowed on hover, so it's clear you can't select the links? (Is there a mobile equivalent?)

While the default calendar behavior doesn't make much sense, I agree we should probably just stick with it for this v1.

@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented Feb 12, 2019

Hi,

I iterated on the styles to get them closer to this version:
calendar block unselected

Current design:
screenshot 2019-02-12 at 21 30 59

This version of the mockup should be possible without any markup change (which if possible it would be better to avoid it for now), so we can continue to iterate on this version.

I can also add some classes used by the calendar widget, and we can go for the calendar widget styles.

Since you can't interact with the calendar months (I forgot about that part), can we do cursor: not-allowed on hover, so it's clear you can't select the links? (Is there a mobile equivalent?)

We are using the Disable component which ensures the user is not able to interact with the calendar (e.g., follow the links) similar to what we do in other blocks e.g.: RSS, latest comments, etc... Should the not-allowed cursor be added at the component level, so all the blocks in a similar situation benefit from it?

@mapk
Copy link
Contributor

mapk commented Feb 12, 2019

All points being made are good. Let's avoid any markup changes and keep the current widget interaction model. I definitely don't want this getting out of hand. But as Mel mentioned above, let's avoid any confusion in the arrows.

@gziolo
Copy link
Member

gziolo commented Feb 14, 2019

I suspect that this is just one of the inherent downsides of using ServerSideRender, though 🙂

Yes, this is a very unfortunate side effect of using ServerSideRender which is a quick way to bring many of the existing widgets into the block editor. I suggested we go this route to have this block available with as little effort as possible for start. I know that @jorgefilipecosta has lots of great ideas on how to further iterate and this is going to be only easier having this PR merged into master. I could find a few another benefits of having early version available in the next Gutenberg release :)

Great work @jorgefilipecosta!

@jorgefilipecosta
Copy link
Member Author

Thank you for the reviews! If someone finds any additional improvement feel free to comment and I will iterate on follow up PR's.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I left some more thoughts about the way we select year and month to consider for the future. I didn't see them as the blocker for the initial version of the PR. It's more my wishful thinking on how to mirror frontend and the editor :)

* @return string Returns the block content.
*/
function render_block_core_calendar( $attributes ) {
global $monthnum, $year, $post;
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, $post is never used. Any reason why phpcs doesn't scream?

getServerSideAttributes( attributes, date ) {
return {
...attributes,
...this.getYearMonth( date ),
Copy link
Member

Choose a reason for hiding this comment

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

I still think it isn't a good approach. Let me explain on the post from my personal website:
https://gziolo.pl/2017/11/07/starter-kit-and-reusable-scripts/
If I visit it as of today you will see that the calendar in the sidebar renders the same month as when this post was published:
screen shot 2019-02-14 at 11 29 11

it behaves differently for the pages as it simply renders the current month:
screen shot 2019-02-14 at 11 31 31

There are also archive pages which follow the URL path:

screen shot 2019-02-14 at 11 33 46

This should also be the case when I want to edit my post in Gutenberg. That's why I suggested that we build logic on PHP side which tries the following:

  • if it is a post type get the publish date from the post metadata
  • if it is a page or custom CPT get the current month

@melchoyce does it make sense?

@jorgefilipecosta
Copy link
Member Author

Hi @gziolo a follow-up PR that addressed the problems raised was opened at #13873. Thank you for your tests 👍

youknowriad pushed a commit that referenced this pull request Mar 6, 2019
The first version of the calendar block.
The design tries to follow the mockup proposed.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
The first version of the calendar block.
The design tries to follow the mockup proposed.
This was referenced Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Feature] Widgets Screen The block-based screen that replaced widgets.php. Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants