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

HTMLCalendar allow custom classes #74281

Closed
oz123 mannequin opened this issue Apr 18, 2017 · 18 comments
Closed

HTMLCalendar allow custom classes #74281

oz123 mannequin opened this issue Apr 18, 2017 · 18 comments
Labels
3.7 stdlib type-feature

Comments

@oz123
Copy link
Mannequin

@oz123 oz123 mannequin commented Apr 18, 2017

BPO 30095
Nosy @doerwalter, @rhettinger, @oz123, @Mariatta
PRs
  • #1439
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2017-06-06.13:04:03.892>
    created_at = <Date 2017-04-18.19:37:57.549>
    labels = ['3.7', 'type-feature', 'library']
    title = 'HTMLCalendar allow custom classes'
    updated_at = <Date 2017-06-06.13:04:03.890>
    user = 'https://github.com/oz123'

    bugs.python.org fields:

    activity = <Date 2017-06-06.13:04:03.890>
    actor = 'doerwalter'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-06-06.13:04:03.892>
    closer = 'doerwalter'
    components = ['Library (Lib)']
    creation = <Date 2017-04-18.19:37:57.549>
    creator = 'Oz.Tiram'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 30095
    keywords = []
    message_count = 18.0
    messages = ['291841', '291858', '291859', '291869', '291872', '291873', '291874', '292155', '292212', '292213', '292236', '292934', '293081', '294846', '294952', '294954', '295245', '295259']
    nosy_count = 4.0
    nosy_names = ['doerwalter', 'rhettinger', 'Oz.Tiram', 'Mariatta']
    pr_nums = ['1439']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue30095'
    versions = ['Python 3.7']

    @oz123
    Copy link
    Mannequin Author

    @oz123 oz123 mannequin commented Apr 18, 2017

    At the moment methods like HTMLCalendar.formatmonthname and HTMLCalendar.formatmonth have hard coded name 'month'.

    This class is pretty helpful as a good start, but if you want to customize
    the styles it's not helpful.

    I think it would be helpful for others too, if this would have be customize able.

    Would you accept a PR for such thing?

    @oz123 oz123 mannequin added stdlib type-feature labels Apr 18, 2017
    @Mariatta
    Copy link
    Sponsor Member

    @Mariatta Mariatta commented Apr 19, 2017

    Can't you override the .month css class and customize the style that way?

    @oz123
    Copy link
    Mannequin Author

    @oz123 oz123 mannequin commented Apr 19, 2017

    Of course I can, but I can't add multiple css classes, and I forced to
    have the same class for both the title of the month and the body of the month.

    @doerwalter
    Copy link
    Contributor

    @doerwalter doerwalter commented Apr 19, 2017

    IMHO this could all be done by overwriting the relevant methods.

    But this might be overkill.

    I think a solution might be to move the CSS classes into class attributes of HTMLCalendar. Customizing the CSS classes would then be done by subclassing HTMLCalendar and overwriting the appropriate class attributes.

    Is this what you had in mind?

    @oz123
    Copy link
    Mannequin Author

    @oz123 oz123 mannequin commented Apr 19, 2017

    @doerwalter, exactly. I found myself overwriting the relevant methods too many times.

    I usually did something like this:

        class WorkCalendar(HTMLCalendar):
    
            def formatmonthname(self, theyear, themonth, withyear=True,
                            style=r'class="month-head"'):
                 
                """
                Return a month name as a table row.
                """
                monthname = super().formatmonthname(theyear, themonth, withyear)
                regex = r'class\="month"'
                return re.sub(regex, style, monthname, 1)

    Using class attributes would nice, also considering that the days CSS classes are defined as class attributes.

    My intention was a few more class attributes (for the month header, and month) and also change the existing code such that each day can have multiple CSS classes and not just one.

    I am willing to work on a PR for that if that sounds good and there is someone who would be willing to merge it.

    @oz123
    Copy link
    Mannequin Author

    @oz123 oz123 mannequin commented Apr 19, 2017

    ... Using class attributes would nice, also considering that the days CSS classes are defined as class attributes.

    I meant to write :

    Using class attributes would be nicer, also considering that the days CSS classes are defined as class attributes.

    @doerwalter
    Copy link
    Contributor

    @doerwalter doerwalter commented Apr 19, 2017

    OK, go ahead. I'm looking forward to what you come up with.

    @oz123
    Copy link
    Mannequin Author

    @oz123 oz123 mannequin commented Apr 23, 2017

    @walter,

    I implemented two possible solutions. Chronologically speaking V2 was implemented before V1, but it's a bit über-smart and might surprise it's
    users requiring the class attributes to be some kind of iterable.

    The first version is my current preferred solution.

    I intentionally didn't create a PR for that yet.

    These are the branches:

    https://github.com/oz123/cpython/tree/issue30095-v1
    https://github.com/oz123/cpython/tree/issue30095-v2

    @doerwalter
    Copy link
    Contributor

    @doerwalter doerwalter commented Apr 24, 2017

    The second link is a 404.

    For the v1 patch:

    The variable names are a bit inconsistent: The first uses "classes" all others use "styles". This should be consistent within itself and with the existing code, i.e. "classes" should be used.

    Also each class attribute should be preceded with a comment, explaining what the CSS class is used for.

    As these are now made public to be overwritten by subclasses, I wonder wether it would make sense to document these class attributes in Doc/library/calendar.rst

    @oz123
    Copy link
    Mannequin Author

    @oz123 oz123 mannequin commented Apr 24, 2017

    @walter,

    I agree with you about consistent naming. Personally, I would prefer to name all the variables "styles". But I preferred not to break past compatibility too. So I guess we are stuck for a while with "classes". I will rename the variables. As for documenting that, I would also do that.
    Thanks for the feedback.

    @oz123
    Copy link
    Mannequin Author

    @oz123 oz123 mannequin commented Apr 24, 2017

    Apparently, I forgot to push the second branch. It is now pushed.

    I renamed the class attribute names on the v1 branch. I still need to do
    the following:

    • Add docs
    • Fix existing tests and add new tests demonstrating the usage of the new attributes.

    @oz123
    Copy link
    Mannequin Author

    @oz123 oz123 mannequin commented May 3, 2017

    @walter,

    I implemented your suggestions plus the obvious tasks of adding tests and documentation.

    Would be happy to hear your feedback.

    @Mariatta Mariatta added the 3.7 label May 3, 2017
    @doerwalter
    Copy link
    Contributor

    @doerwalter doerwalter commented May 5, 2017

    See comments on Github

    @doerwalter
    Copy link
    Contributor

    @doerwalter doerwalter commented May 31, 2017

    See my comments on the pull request: #1439

    After you address those, IMHO this is ready to be merged.

    @doerwalter
    Copy link
    Contributor

    @doerwalter doerwalter commented Jun 1, 2017

    See comments on the pull request. Also it seems that currently the pull request can't be merged because of merge conflicts.

    @oz123
    Copy link
    Mannequin Author

    @oz123 oz123 mannequin commented Jun 1, 2017

    @walter, I fixed the issues your raised, and also solved the merge conflict in What's New.

    @doerwalter
    Copy link
    Contributor

    @doerwalter doerwalter commented Jun 6, 2017

    New changeset 8b7a4cc by Walter Dörwald (Oz N Tiram) in branch 'master':
    bpo-30095: Make CSS classes used by calendar.HTMLCalendar customizable (GH-1439)
    8b7a4cc

    @doerwalter
    Copy link
    Contributor

    @doerwalter doerwalter commented Jun 6, 2017

    Closing the issue. The patch has been merged.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 stdlib type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants