I've created a template for date_view module (that is part of date module) and that is used in calendar module for styling the calendar. As calendar module is propular like webforms it should be added to the theme.

The buttons on top left are improperly aligned and look totally broken without this template. See the screenshots attached.

Before:

After

Comments

hass created an issue. See original summary.

hass’s picture

Status: Active » Needs review
FileSize
2.57 KB
markcarver’s picture

Status: Needs review » Needs work
  1. +++ b/templates/date_views/date-views-pager.tpl.php
    

    The folder beneath templates should just be date (not date_view) since that is the project name this module is bundled with.

  2. +++ b/templates/date_views/date-views-pager.tpl.php
    @@ -0,0 +1,64 @@
    + * @file
    + * Template file for the bootstrap theme display.
    

    Incorrect/inadequate description of what this template file is for.

  3. +++ b/templates/date_views/date-views-pager.tpl.php
    @@ -0,0 +1,64 @@
    + * Variables available:
    + *
    + * $plugin: The pager plugin object. This contains the view.
    

    These variables should follow existing core standards: https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

    It's also a couple missing variables being used in the template below that aren't described here: $pager_prefix (no suffix?) and $extra_classes.

  4. +++ b/templates/date_views/date-views-pager.tpl.php
    @@ -0,0 +1,64 @@
    + */
    +?>
    

    Missing @ingroup templates at the bottom of the file's doxygen comment. Despite what the above link says, this is necessary for http://drupal-bootstrap.org to pick up and categorize this file properly.

  5. +++ b/templates/date_views/date-views-pager.tpl.php
    @@ -0,0 +1,64 @@
    +    <div class="col-xs-12 col-md-4 col-md-offset-4">
    

    Why is this using col-md-offset-4? Please tell me this isn't to "align" the text in the center.

    I'm assuming this is the month? If so, it should just be using the text-center class.

  6. +++ b/templates/date_views/date-views-pager.tpl.php
    @@ -0,0 +1,64 @@
    +        <h3><?php print $nav_title ?></h3>
    

    Is $nav_title already sanitized?

  7. +++ b/templates/date_views/date-views-pager.tpl.php
    @@ -0,0 +1,64 @@
    +    <div class="col-xs-12 col-md-4">
    

    Should be a <nav> element since there's a <ul> element immediately inside it.

    This should also be before the header so it can be floated to the right properly on larger screens.

  8. +++ b/templates/date_views/date-views-pager.tpl.php
    @@ -0,0 +1,64 @@
    +          <?php
    +          $text = '&laquo;';
    +          $text .= $mini ? '' : ' ' . t('Prev', array(), array('context' => 'date_nav'));
    +          $aria_type = $mini ? 'aria-label' : 'aria-labelledby';
    +          $prev_options['attributes'][$aria_type] = t('Prev', array(), array('context' => 'date_nav'));
    +          print l(t($text), $prev_url, $prev_options);
    +          ?>
    ...
    +          <?php
    +          $next_options['attributes'][$aria_type] = t('Next', array(), array('context' => 'date_nav'));
    +          print l(($mini ? '' : t('Next', array(), array('context' => 'date_nav')) . ' ') . '&raquo;', $next_url, $next_options);
    +          ?>
    

    Um. No.

    All this should be done in a preprocess function, creating new variables if necessary (to compensate for module's laziness of proper theming I'm assuming).

    Furthermore, it should be using _bootstrap_icon() with something like menu-left instead of &laquo; and menu-right instead of &raquo;.

    Finally, we shouldn't be passing a variable to t() just to pass to l(), especially since it's already been translated above that.

hass’s picture

Why is this using col-md-offset-4? Please tell me this isn't to "align" the text in the center. I'm assuming this is the month? If so, it should just be using the text-center class.

Correct. The month name need to be aligned to the center. text-center does not work as the buttons on the right will move the month outside the center to the left. Additionally on small devices the prev/next buttons need to move to the next line so that month and buttons are both centered and stacked above with each other. I wish we would have float-xs-right in bootstrap 3, but this is v4 only.

If you have a solution how to solve this I'm happy to improve the code.

markcarver’s picture

I already did.

hass’s picture

What you suggested does not work.

markcarver’s picture

Never mind... I was thinking of something else.

hass’s picture

It's also a couple missing variables being used in the template below that aren't described here: $pager_prefix (no suffix?) and $extra_classes.

Date module does not document all these params. This is not my code. The closer it is to the original the easier it is to maintain. I reviewed the date module, but have not found the vars. I'm not sure what module may add these vars. We should wait until D8 calendar is available and see how the templates are changing then.

Is $nav_title already sanitized?

I have not made a security review of date module if you are asking for this. If it would not, we may have seen a security release of date module in past. But it looks to be safe.

All this should be done in a preprocess function, creating new variables if necessary (to compensate for module's laziness of proper theming I'm assuming).

It could, but it must not. See the original file at http://cgit.drupalcode.org/date/tree/date_views/theme/date-views-pager.t...

Furthermore, it should be using _bootstrap_icon() with something like menu-left instead of « and menu-right instead of ».

This are valid signs. See https://getbootstrap.com/components/#pagination that is also using them. I attached the standard and the gly one, but prefer the standard as the gly one is not looking so great.

Fixed all the other fixable things that makes sense.

markcarver’s picture

Status: Needs review » Needs work
hass’s picture

This patch needs no work.

markcarver’s picture

The above patch is not up to par with the quality of code that is currently in this base theme.

It will not be committed as is.

Like many other templates, complex variables (display logic), is done in preprocess functions.

Please follow the existing workflow and structure of this base theme.

This patch needs no work.

You're not the maintainer of this project. I outlined what needed to be fixed above and you ignored what you deemed "didn't make sense".

That's not how contrib works and certainly not how I work.

hass’s picture

It looks like the rules you are making are not for the project and only for me. I see http://cgit.drupalcode.org/bootstrap/tree/templates/system/pager.func.ph... that shows that the pagination is a <DIV> and not a <NAV>. Additionally they are using &laquo; and &raquo; and others and not the _bootstrap_icon(). This means the patch above with _bootstrap_icon() must not used for consistency reasons.

It looks like all other tpl.php files are such type of code. format_plural in a tpl.php. No idea what should be changed now as all is clean..

markcarver’s picture

No I'm not "making up rules just for you". The code you reference already exists (long before I made this project into what it is). Would I like to change it, sure, but I can't. That is code that has been out for some time and cannot be changed because it'd introduce a BC break.

This patch is introducing new code. Code that should, regardless of where it came from, utilize a preprocess function for all that PHP logic (which is mostly what I was referring to).

Furthermore, you seem to keep forgetting that 7.x-3.x isn't HEAD; 8.x-3.x is and it's using <nav>:

http://drupal-bootstrap.org/api/bootstrap/templates%21system%21pager.htm...

Regarding the icons, no, neither 8.x-3.x or 7.x-3.x are using them, specifically for the pager, but that's really not the point. This isn't a pager. It's a different theme hook/template all together. One that you're making look like a pager, nothing more.

If I could go back and put icons in the pagers, I would. It just makes it look sharper and also helps with accessibility.

---

I was attempting to give you the benefit of the doubt since you at least provided a patch.

I was originally tempted to mark this as "Closed (won't fix)" solely for the following reasons:

  1. The fact that this is against 7.x-3.x
  2. The fact that no one, even remotely, has mentioned needing a template for this from the base theme.
  3. This is a template for a contrib module that not everyone will have (despite it appearing to be "popular").

However, since you already provided a patch and this is just an addition, I gave feedback.

You implemented some changes and ignored some feedback entirely.

I'm really not an unreasonable guy, but if this is how you're going to be then I have no qualms about rejecting this issue based solely on your attitude and the above reasons.

hass’s picture

This is a pager. 100%. You may like to install calendar module and take a look into the view. It pice of code is named pager everywhere in views/date/calendar code.

My attitude is to fix issues that affect me and others.

hass’s picture

Status: Needs review » Needs work
hass’s picture

Status: Needs work » Needs review
FileSize
3.75 KB
hass’s picture

markcarver’s picture

This is a pager. 100%. You may like to install calendar module and take a look into the view. It pice of code is named pager everywhere in views/date/calendar code.

No, they're just navigation links, so probably ~16% a "pager". A true pager (historically) also has numbers indicating the actual page you are currently on and subsequent pages you can navigate to before and after.

Just because they use the term "pager" through out the code doesn't actually mean that it's a "pager". They could easily, just as well, be just simple inline links.

My attitude is to fix issues that affect me and others.

No, your attitude has been to undermine and berate this issue queue whenever something you don't find appealing to your satisfaction. It's actually been quite a rude experience.

---

Now, to actually get back to the code. I was going to add another lengthy list of reviews, but I figured it's just be easier to code it so you can see what I mean.

Note: this does require the latest dev as the following was just backported to 7.x-3.x #2840791: Add icon support to "link" theme hook.

  • markcarver committed 230ef65 on 7.x-3.x
    Issue #2839880 by markcarver, hass: Add date/calendar module template
    
markcarver’s picture

Status: Needs review » Fixed
hass’s picture

Heavy patch, but ok.

  1. This introduced a new string Previous with context that does not exists in any language. Keep in mind that Prev is translated in tons of languages and this cause a translation break for users. This is not a good idea in the middle of D7. I agree it should not abbreviated, but this is a different story we may need to discuss with Calendar team. This new string should be rolled back.
  2. Mini pagers are not supported. Why?
markcarver’s picture

  1. I could have sworn that it falls back to a non-context specific translation if a specific context translation doesn't exist. However, I just re-read locale() and it's not that fancy. Sigh... "Prev" isn't an English word, it's an abbreviation for "Previous". I also looked up the translations for this context and you're right, it's all for "Prev"... I'll revert the string, sad.
  2. Mini pagers are supported, they only show the icon. See http://drupal-bootstrap.org/api/bootstrap/templates%21date%21date-views-...

  • markcarver committed f37937f on 7.x-3.x
    Revert to "Prev" translation in date_views_pager
    
    Issue #2839880 by...
markcarver’s picture

I created this related follow-up.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.