Skip to content

Conversation

@oz123
Copy link
Contributor

@oz123 oz123 commented May 3, 2017

as discussed here http://bugs.python.org/issue30095

Unfortunately, this whole module isn't pep8 compatible, I didn't touch where I am not supposed to touch, so pep8 won't passed here.

oz123 added 4 commits April 23, 2017 08:57
The HTMLCalendar has now multiple class attributes for styling the
calendar.
In order to retain backwards compatibility style names are not changed.

The new attributes are:

  weekday_head_styles
  month_head_styles = ("month",)
  year_head_styles = ("year",)
  year_styles = ("year",)

This are added to the previously existing styles, which where scattered
around the code hard coded or as class attributes:

  month style
  day styles
  noday style

The relevant methods have been modified to use these attributes. If you
want a calendar where elements has multiple CSS attributes you can
now do the following:

>>> my_styles = HTMLCalendar.cssclasses[:]
>>> my_stelys = " ".join((s, "black text-centered")) for s in my_styles]

class MyCalendar(HTMLCalendar):
   cssclasses = styles
   month_head_styles = "month-title text-bold"

This implementation simply replaces hard coded string with class
attributes (which are also single strings albite the attribute names which are
in plural).
@mention-bot
Copy link

@oz123, thanks for your PR! By analyzing the history of the files in this pull request, we identified @doerwalter, @Yhg1s and @birkenfeld to be potential reviewers.

@Mariatta Mariatta added the type-feature A feature request or enhancement label May 3, 2017
 Fix typos, and more class attributes documentation
@oz123
Copy link
Contributor Author

oz123 commented May 8, 2017

@doerwalter, I updated the documentation. I would be happy to hear your comments now.

sheet should be used. *encoding* specifies the encoding to be used for the
output (defaulting to the system default encoding).

:class:`HTMLCalendar` has the following attribute you can override to customize the style of your calender:
Copy link
Contributor

Choose a reason for hiding this comment

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

attribute -> attributes
Try not to use 'you' or 'your'.


.. attribute:: cssclasses

Is a list of CSS styles used for each weekday. The default style list is::
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this should start with 'is'. The calendar attributes on this page all start with a subject, such as 'An array'.


cssclasses = ["mon text-bold", "tue", "wed", "thu", "fri", "sat", "sun red"]

Note, that the length of this list must be 7 items.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this needs a comma after Note.


.. attribute:: noday_classes

The CSS class for a week day occuring in previous or the coming month.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • occuring -> occurring.
  • week day -> weekday
  • "in the previous or coming month"


Is a list of CSS styles used for each weekday. The default style list is

the same as ``HTMLCalendar.cssclassess``.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Not sure about starting with 'is'.
  • These two lines gave odd spacing when I looked at the html rendered.
  • There's an extra 's' at the end of HTMLCalendar.cssclasses.


.. attribute:: month_head_classes

A space separted list of styles for the month head, for example::
Copy link
Contributor

Choose a reason for hiding this comment

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

separted -> separated


.. attribute:: month_classes

a space separted list of styles for the whole table.
Copy link
Contributor

@csabella csabella May 10, 2017

Choose a reason for hiding this comment

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

  • Capitalize 'a'.
  • separted -> separated
  • Also, there is a blank line in between the description and the default value for this, year_classes and year_head_classes.

.. attribute:: year_classes

a space separted list of styles for the table when formatting the year
as a table of tables (see: ``HTMLCalendar.formatyear``).
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Capitalize 'a'.
  • separted -> separated


.. attribute:: year_head_classes

a space separted list of styles for the table head when formatting the year as a table of tables (see: ``HTMLCalendar.formatyear``).
Copy link
Contributor

Choose a reason for hiding this comment

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

  • capitalize 'a'
  • separted -> separated

@csabella
Copy link
Contributor

When I looked at the html page for the docs, the indentation was off compared to the section before your changes (I used 'make html' to be able to view it). I'm also wondering about the name of 'cssclasses'. Since the year and month ones have year and month in the name, maybe this should too?

@oz123
Copy link
Contributor Author

oz123 commented May 10, 2017

@csabella Thanks for the thorough review. I am embarrassed there are so many spelling errors...

I would prefer to rename the cssclasses attribute to weekday_classes, but I was afraid somebody is already using it with that name.
If there is no objection to that, I would rename that attribute. I will correct the spelling errors as soon as possible.

@csabella
Copy link
Contributor

@oz123 Please don't be embarrassed about it. I probably should have just asked you to spellcheck it instead of commenting on each one. Sorry about that.

As far as the attribute name, maybe @doerwalter could help resolve that. I just found it inconsistent when reading the doc, so I wanted to ask about it.

@oz123
Copy link
Contributor Author

oz123 commented May 10, 2017

@doerwalter what do you think about renaming the attribute HTMLCalendar.cssclassesto HTMLCalendar.weekday_styles`?

@csabella
Copy link
Contributor

@oz123 I looked at the html again and still had the issue with the odd indentation. Looking at the source, I noticed you used 4 spaces (or tab) to indent. The docs use 3 spaces (and no tabs) as the standard with an 80 character limit.

Here's the reference in the dev guide for formatting the docs:
https://docs.python.org/devguide/documenting.html#style-guide

@oz123
Copy link
Contributor Author

oz123 commented May 10, 2017

@csabella fixed the white space. Can you check now?

output (defaulting to the system default encoding).

:class:`HTMLCalendar` has the following attributes you can override to
customize the style of your calender:
Copy link
Contributor

Choose a reason for hiding this comment

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

calender -> calendar


cssclasses = ["mon text-bold", "tue", "wed", "thu", "fri", "sat", "sun red"]

Note that the length of this list must be 7 items.
Copy link
Contributor

Choose a reason for hiding this comment

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

Other places in this page use 'seven' instead of '7'.


.. attribute:: noday_classes

The CSS class for a week day occurring in previous or the coming month.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • week day -> weekday
  • try "in the previous or coming month"

@doerwalter
Copy link
Contributor

Also you might want to add an appropriate description of the new features to Doc/whatsnew/3.7.rst.

@oz123
Copy link
Contributor Author

oz123 commented Jun 1, 2017

@doerwalter I added the changes you suggested.
I really hope these additions to HTMLCalendar will be useful for others too.
Thank you for all the support and suggestions until now.


Note that the length of this list must be seven items.

.. versionadded:: 3.7
Copy link
Contributor

Choose a reason for hiding this comment

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

The .. versionadded:: 3.7 should go inside the .. attribute:: block (after the description), i.e.

    .. attribute:: cssclass_noday
  
        The CSS class for a weekday occurring in the previous or coming month.

        .. versionadded:: 3.7

Note that you can create the documentation yourself to check the result via make html in the Doc directory (you need Sphinx for that)

.. versionadded:: 3.7
.. attribute:: cssclass_month_head

The month's head CSS class. The default value is ``"month"``.
Copy link
Contributor

Choose a reason for hiding this comment

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

You could add a reference to formatmonthname here, i.e.:

The month's head CSS class (see :meth:`formatmonthname`). The default value is ``"month"``.

.. versionadded:: 3.7
.. attribute:: cssclass_month

The CSS class for the whole month's table. The default value is ``"month"``.
Copy link
Contributor

Choose a reason for hiding this comment

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

You could add a reference to formatmonth here, i.e.:

The CSS class for the whole month's table (see :meth:`formatmonth`). The default value is ``"month"``.

customisation the CSS classes in the produced HTML calendar.
(Contributed by Oz Tiram in :issue:`30095`.)


Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO this should be something like this:

calendar
--------

CSS classes used by :class:`~calendar.HTMLCalendar` can now be customized by
subclassing :class:`~calendar.HTMLCalendar` and overwriting certain class
attributes. (Contributed by Oz Tiram in :issue:`30095`.)

def test_formatweek(self):
weeks = self.cal.monthdays2calendar(2017,5)
self.assertIn('class="wed text-nowrap"', self.cal.formatweek(weeks[0]))
def test_format_year(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a newline missing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 45d68c2

@doerwalter
Copy link
Contributor

I pushed a small documentation fix to your branch.

@oz123
Copy link
Contributor Author

oz123 commented Jun 1, 2017

@doerwalter WOW! I didn't know you can push to a pull request! That is cool.

@doerwalter
Copy link
Contributor

@doerwalter
Copy link
Contributor

I've renamed cssclass_weekday_head to cssclasses_weekday_head, since this is a list of names.

I've noticed that there's no test that checks for this configuration. Could you add one? And if there are other tests missing add those too.

@oz123
Copy link
Contributor Author

oz123 commented Jun 5, 2017

@doerwalter I think the change you introduce is good. I also added tests to all the class attributes. Hopefully, this is the last iteration.

@doerwalter doerwalter merged commit 8b7a4cc into python:master Jun 6, 2017
@doerwalter
Copy link
Contributor

OK, your patch is in.

Congratulations and thanks for your contribution to Python.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants