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 styles for gists #295

Merged
merged 15 commits into from
Aug 5, 2024
Merged

Add styles for gists #295

merged 15 commits into from
Aug 5, 2024

Conversation

chris-lawton
Copy link
Member

@chris-lawton chris-lawton commented Jul 24, 2024

Link to Ticket

Description of Changes Made

Styles for codehilite have been added to the build but these only kick in when a programming language is added in code block streamfield.

When a link to a github gist is added via a raw HTML streamfileld then different classes are used for styling, not codehilite.

I've found a github repo where gist's are styled so have added that to the build. After doing so there are no colour contrast issues being reported by aXe.

How to Test

Screenshots

Before

Screenshot 2024-07-24 at 10 45 36

After

Screenshot 2024-07-24 at 10 45 09

MR Checklist

  • Add a description of your pull request and instructions for the reviewer to verify your work.
  • If your pull request is for a specific ticket, link to it in the description.
  • Stay on point and keep it small so the merge request can be easily reviewed.
  • Tests and linting passes.

Unit tests

  • Added
  • Not required

Documentation

Browser testing

  • I have tested in the following browsers and environments (edit the list as required)
    • Latest version of Chrome on mac
    • Latest version of Firefox on mac
    • Latest version of Safari on mac
    • Safari on last two versions of iOS
    • Chrome on last two versions of Android
  • Not required

Data protection

  • Not relevant
  • This adds new sources of PII and documents it and modifies Birdbath processors accordingly

Accessibility

  • Automated WCAG 2.1 tests pass
  • HTML validation passes
  • Manual WCAG 2.1 tests completed
  • I have tested in a screen reader
  • I have tested in high-contrast mode
  • Any animations removed for prefers-reduced-motion
  • Not required

Sustainability

  • Images are optimised and lazy-loading used where appropriate
  • SVGs have been optimised
  • Perfomance and transfer of data considered
  • If JavaScript is needed alternatives have been considered
  • Not required

Pattern library

  • The pattern library component for this template displays correctly, and does not break parent templates
  • The styleguide is updated if relevant
  • Changes are not relevant the pattern library

@helenb
Copy link
Member

helenb commented Jul 25, 2024

This is a good fix, thanks chris. I wonder though if there is a way to only include the css if there is a gist linked on the page, as this is a lot of css to add to every page. I will have a think.

@helenb
Copy link
Member

helenb commented Jul 25, 2024

This is a good fix, thanks chris. I wonder though if there is a way to only include the css if there is a gist linked on the page, as this is a lot of css to add to every page. I will have a think.

I think something like this could work, but you'd need probably need to convert the gist scss file to css, and add a new entry for it in webpack:

{% load static %}
<div class="grid__raw-html">
    {{ value }}
    {% if 'gist' in value %}
        <link rel="stylesheet" type="text/css" href="{% static 'css/vendor/gist.css' %}">
    {% endif %}
</div>

@chris-lawton
Copy link
Member Author

chris-lawton commented Jul 26, 2024

This is a good fix, thanks chris. I wonder though if there is a way to only include the css if there is a gist linked on the page, as this is a lot of css to add to every page. I will have a think.

I think something like this could work, but you'd need probably need to convert the gist scss file to css, and add a new entry for it in webpack:

{% load static %}
<div class="grid__raw-html">
    {{ value }}
    {% if 'gist' in value %}
        <link rel="stylesheet" type="text/css" href="{% static 'css/vendor/gist.css' %}">
    {% endif %}
</div>

Thanks @helenb - that's a nice idea. Reckon we should do the same for the codehilite css as well (if there's a RAW HTML streamfield on the page)?

{% load wagtailcore_tags wagtailimages_tags static %}
{% load wagtailcore_tags wagtailimages_tags util_tags static %}

{% block extra_css %}
Copy link
Member

Choose a reason for hiding this comment

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

btw, I believe the Work page also uses StoryBlock so can in theory have these

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, doesn't seem to work directly on the work_page.html - I guess it's because the streamfields are nested on this page and are not directly in page.body?

Copy link
Member

Choose a reason for hiding this comment

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

it seems to work if you add the extra_css block in work_page_base.html 🤷🏼

Copy link
Member

Choose a reason for hiding this comment

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

StoryBlock is used by:

  • blog page
  • standard page
  • impact report page
  • service page
  • work page
  • historical work page

We could add it to all of these, or possibly just add the check in the base template.

Copy link
Member

Choose a reason for hiding this comment

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

Markdown is also in the StoryBlock so available in all the same page templates.

Copy link
Member Author

@chris-lawton chris-lawton Jul 29, 2024

Choose a reason for hiding this comment

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

From Helens debugging it seems that the streamfields are nested within Sections on the Work pages so the filter as it is right now doesn't work on work_base_page.html

@register.filter(name="has_markdown_block")
def has_markdown_block(value):
    print('running')
    if not isinstance(value, StreamValue):
        return False

    print('still running')
    for block in value._raw_data:
        print(block["type"])
        if block["type"] == "markdown":
            print('found markdown')
            return True

On work pages this always prints 'Section' so I guess we'd need to adjust the filter to search within the sections rather than the page.body?

Copy link
Member Author

@chris-lawton chris-lawton Jul 29, 2024

Choose a reason for hiding this comment

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

Helen helped out on Slack and the filter been updated to search nested streamfields within sections and it's now been added to base.html - ca994a5

Next: add docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

@zerolab If you have a sec to cast an eye over the changes here it would be much appreciated! 6e87ff6

Copy link
Member

@helenb helenb left a comment

Choose a reason for hiding this comment

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

I am a bit confused, given that the extra CSS hasn't been added to the standard page, that I am seeing the exact same styles for a gist on a standard page as on a blog page:

Blog page:

Screenshot 2024-07-29 at 11 25 54

Standard page:

Screenshot 2024-07-29 at 11 25 50

tbx/core/templatetags/util_tags.py Outdated Show resolved Hide resolved
{% load wagtailcore_tags wagtailimages_tags static %}
{% load wagtailcore_tags wagtailimages_tags util_tags static %}

{% block extra_css %}
Copy link
Member

Choose a reason for hiding this comment

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

StoryBlock is used by:

  • blog page
  • standard page
  • impact report page
  • service page
  • work page
  • historical work page

We could add it to all of these, or possibly just add the check in the base template.

tbx/static_src/sass/vendor/codehilite.scss Show resolved Hide resolved
@helenb
Copy link
Member

helenb commented Jul 29, 2024

I am a bit confused, given that the extra CSS hasn't been added to the standard page, that I am seeing the exact same styles for a gist on a standard page as on a blog page:

CSS loaded on standard:

Screenshot 2024-07-29 at 11 28 36

CSS loaded on a blog page:

Screenshot 2024-07-29 at 11 29 34

@helenb
Copy link
Member

helenb commented Jul 29, 2024

I am a bit confused, given that the extra CSS hasn't been added to the standard page, that I am seeing the exact same styles for a gist on a standard page as on a blog page:

Please ignore all the above! I forgot I needed to restart the tooling as webpack had been updated! doh!

@helenb
Copy link
Member

helenb commented Jul 29, 2024

@chris-lawton these changes could do with some documentation.

@chris-lawton chris-lawton force-pushed the fix/code-embed-colour-contrast branch from dfbf46a to f53aa9b Compare July 29, 2024 14:42
@chris-lawton
Copy link
Member Author

@chris-lawton these changes could do with some documentation.

Good call. Once i've figured out how they're added to the pages then i'll add in 👍

@chris-lawton
Copy link
Member Author

@helenb Docs added here: af4e434

@helenb
Copy link
Member

helenb commented Jul 30, 2024

Docs changes looked great.

I just tested again with a work page, and although the code works fine on a published page, unfortunately I'm still seeing an error in the preview. So suspect the code needs some further tweaking which I'm hoping @zerolab might be able to have some thoughts on.

Screenshot 2024-07-30 at 14 32 49

@zerolab
Copy link
Member

zerolab commented Jul 30, 2024

@helenb should be fixed in c038dec

@helenb
Copy link
Member

helenb commented Aug 1, 2024

@helenb should be fixed in c038dec

Great - tested and all working well.

@chris-lawton chris-lawton merged commit ce00cad into main Aug 5, 2024
6 checks passed
@chris-lawton chris-lawton deleted the fix/code-embed-colour-contrast branch August 5, 2024 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants