Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

feat: promo video on homepage (#7229) #7230

Merged
merged 3 commits into from
May 30, 2023
Merged

feat: promo video on homepage (#7229) #7230

merged 3 commits into from
May 30, 2023

Conversation

eddiejaoude
Copy link
Member

@eddiejaoude eddiejaoude commented May 28, 2023

Fixes Issue

fixes #7229

Screenshot 2023-05-28 at 12 07 05

Changes proposed

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

Note to reviewers

@github-actions github-actions bot added large Pull request with more than 30 changed lines waiting-for-reviewers labels May 28, 2023
@github-actions
Copy link
Contributor

Reviewpad Report

ℹ️ Messages

  • A maintainer will review your pull request soon!

@eddiejaoude
Copy link
Member Author

Anyone know how to fix this failing test with the YouTube embed video

Running 5 tests using 1 worker
[WebServer] - warn Fast Refresh had to perform a full reload. Read more: https://nextjs.org/docs/messages/fast-refresh-reload
[WebServer] (node:5001) [DEP0128] DeprecationWarning: Invalid 'main' field in '/Users/ej/Downloads/repos/EddieHub/LinkFree/node_modules/react-icons/package.json' of 'lib'. Please either fix that or report it to the module author
(Use `node --trace-deprecation ...` to show where the warning was created)
[WebServer] - warn Fast Refresh had to perform a full reload. Read more: https://nextjs.org/docs/messages/fast-refresh-reload
  1) [chromium] › home.spec.js:31:7 › accessibility tests (light) › should pass axe wcag accessibility tests (light) 

    Error: expect(received).toEqual(expected) // deep equality

    - Expected  -  1
    + Received  + 44

    - Array []
    + Array [
    +   Object {
    +     "description": "Ensures ARIA attributes are allowed for an element's role",
    +     "help": "Elements must only use allowed ARIA attributes",
    +     "helpUrl": "https://dequeuniversity.com/rules/axe/4.7/aria-allowed-attr?application=playwright",
    +     "id": "aria-allowed-attr",
    +     "impact": "serious",
    +     "nodes": Array [
    +       Object {
    +         "all": Array [],
    +         "any": Array [],
    +         "failureSummary": "Fix all of the following:
    +   aria-label attribute cannot be used on a a with no valid role attribute.",
    +         "html": "<a class=\"ytp-title-channel-logo\" target=\"_blank\" aria-label=\"Photo image of Eddie Jaoude\" style=\"background-image: url(&quot;https://yt3.ggpht.com/AYrAjbqBS43Ocd0FMbrM0f27kwqKCjh46Pi8tPVIGdN8IIy4oInLDVVfNNyU4Wmtjw2mbkq3=s68-c-k-c0x00ffffff-no-rj&quot;);\"></a>",
    +         "impact": "serious",
    +         "none": Array [
    +           Object {
    +             "data": Object {
    +               "messageKey": "noRoleSingular",
    +               "nodeName": "a",
    +               "prohibited": Array [
    +                 "aria-label",
    +               ],
    +               "role": null,
    +             },
    +             "id": "aria-prohibited-attr",
    +             "impact": "serious",
    +             "message": "aria-label attribute cannot be used on a a with no valid role attribute.",
    +             "relatedNodes": Array [],
    +           },
    +         ],
    +         "target": Array [
    +           "iframe",
    +           ".ytp-title-channel-logo",
    +         ],
    +       },
    +     ],
    +     "tags": Array [
    +       "cat.aria",
    +       "wcag2a",
    +       "wcag412",
    +     ],
    +   },
    + ]

      34 |       .withTags(["wcag2a", "wcag2aa", "wcag21a", "wcag21aa"])
      35 |       .analyze();
    > 36 |     expect(accessibilityScanResults.violations).toEqual([]);
         |                                                 ^
      37 |   });
      38 | });
      39 |

        at /Users/ej/Downloads/repos/EddieHub/LinkFree/tests/home.spec.js:36:49

    attachment #1: trace (application/zip) ─────────────────────────────────────────────────────────
    test-results/home-accessibility-tests-light-should-pass-axe-wcag-accessibility-tests-light--chromium/trace.zip
    Usage:

        npx playwright show-trace test-results/home-accessibility-tests-light-should-pass-axe-wcag-accessibility-tests-light--chromium/trace.zip

    ────────────────────────────────────────────────────────────────────────────────────────────────

@kumarsonsoff3
Copy link
Member

kumarsonsoff3 commented May 28, 2023

The issue seems to be related to the accessibility of iframe element. Since "aria-label attribute cannot be used on a with no valid role attribute."

We can either remove the aria-label attribute, as the title already provides the required information.
Or add a role="video" attribute to the iframe element.

@eddiejaoude
Copy link
Member Author

eddiejaoude commented May 28, 2023

The issue seems to be related to the accessibility of iframe element. Since "aria-label attribute cannot be used on a with no valid role attribute."

We can either remove the aria-label attribute, as the title already provides the required information. Or add a role="video" attribute to the iframe element.

Thank you 👍 I think it is not on the iframe though but an item with the iframe because removing aria-label or adding role="video" I get the same error?

<a class="ytp-title-channel-logo" target="_blank" aria-label="Photo image of Eddie Jaoude" style="background-image: url("https://yt3.ggpht.com/AYrAjbqBS43Ocd0FMbrM0f27kwqKCjh46Pi8tPVIGdN8IIy4oInLDVVfNNyU4Wmtjw2mbkq3=s68-c-k-c0x00ffffff-no-rj&quot;);\">",

@dan-mba
Copy link
Member

dan-mba commented May 28, 2023

It is a YouTube embed problem. There is no way you can fix it outside of not using the iframe API.

There used to be an option to not show channel info before play started, but they deprecated it.

@kumarsonsoff3
Copy link
Member

One temporary solution could be to manipulate the test to ignore the violation of this specific issue but this might not be a good practice & may affect the accessibility of the project.

@dan-mba
Copy link
Member

dan-mba commented May 28, 2023

If YouTube isn't necessary, I would suggest just using a mp4 video file.

@eddiejaoude
Copy link
Member Author

Thanks for the info and suggestions 👍

YouTube is not needed, we could do mp4, but the benefit is that YouTube does the different resolutions for different internet speeds.

So I guess the options are:

  • get the test to ignore that failure (not sure if possible?)
  • use mp4 instead of YouTube

I am not sure which is better?

@dan-mba
Copy link
Member

dan-mba commented May 29, 2023

Thanks for the info and suggestions 👍

YouTube is not needed, we could do mp4, but the benefit is that YouTube does the different resolutions for different internet speeds.

So I guess the options are:

* get the test to ignore that failure (not sure if possible?)

* use mp4 instead of YouTube

I am not sure which is better?

One possible solution is using a video hosting solution like Cloudinary.
From what I can tell, if you use it just for the 1 video & it is not heavily streamed, you should be able to get by on the free plan.

@eddiejaoude
Copy link
Member Author

Thanks Dan for those suggestions 👍 I found out our platform we use for EddieHub supports videos and the tests seem to now pass - so I will use that for now

@eddiejaoude eddiejaoude merged commit 182e1d2 into main May 30, 2023
@eddiejaoude eddiejaoude deleted the issue-7229 branch May 30, 2023 04:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
large Pull request with more than 30 changed lines waiting-for-reviewers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] replace promotional screenshots with a promotional video
3 participants