Skip to content

✏️(frontend) fix GitHub capitalization #1274

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

karlhorky
Copy link

@karlhorky karlhorky commented Aug 7, 2025

Purpose

Fix capitalization of the brand GitHub

Proposal

  • Fix capitalization of GitHub

External contributions

Thank you for your contribution! 🎉

Please ensure the following items are checked before submitting your pull request:

  • I have read and followed the contributing guidelines
  • I have read and agreed to the Code of Conduct
  • I have signed off my commits with git commit --signoff (DCO compliance)
  • I have signed my commits with my SSH or GPG key (git commit -S)
  • My commit messages follow the required format: <gitmoji>(type) title description
  • I have added a changelog entry under ## [Unreleased] section (if noticeable change)
  • I have added corresponding tests for new features or bug fixes (if applicable)

@karlhorky karlhorky force-pushed the patch-1 branch 5 times, most recently from 17f452f to 7931013 Compare August 8, 2025 09:25
@karlhorky karlhorky changed the title Fix GitHub capitalization ✏️(frontend) fix GitHub capitalization Aug 8, 2025
@karlhorky
Copy link
Author

karlhorky commented Aug 8, 2025

@AntoLC looking at the CI failures:

  1. 3: B6 Body message is missing https://github.com/suitenumerique/docs/actions/runs/16801989694/job/47633068291
  2. Check that the CHANGELOG has been modified in the current branch https://github.com/suitenumerique/docs/actions/runs/16801989694/job/47633068314

For 1, not sure if this commit format is correct?

For 2, I was trying to understand what it is trying to tell me. At first glance, it appears to want a changelog entry for this PR. However, looking at the workflow, it shows an exception for PRs with noChangeLog labels.

Should this PR have this noChangelog label? Or should I actually add a changelog entry for this typo fix?

@virgile-dev
Copy link
Collaborator

Hello @karlhorky
Thanks for your contribution.

  1. Your commit needs to contain a description (It shouldn’t be too long and explain why you made the change). See how to : https://stackoverflow.com/questions/16122234/how-to-commit-a-change-with-both-message-and-description-from-the-command-li
  2. This one is not mandatory so you can ignore it especially since it’s a small change.

@karlhorky
Copy link
Author

karlhorky commented Aug 21, 2025

@virgile-dev thanks for the tips!

  1. It does already contain a description, no? The message from commit 7931013 (#1274)
    ✏️(frontend) fix GitHub capitalization
    
    The capitalization of the product GitHub has a capital H
    
    The CI didn't run again though, it probably neds to be approved.
  2. Ok, thanks

Maybe the CI run just needs to be approved again and this can pass and be reviewed + merged?

@virgile-dev
Copy link
Collaborator

@karlhorky
Copy link
Author

karlhorky commented Aug 22, 2025

@virgile-dev Ok, I read that requirement quickly and I thought it was related to GitHub commit signing - the commit I mentioned above was signed:

Screenshot 2025-08-22 at 10 11 10

But maybe "signoffs" are different than "signing". I've used the --signoff flag and amended the commit, here's the new one:

@karlhorky
Copy link
Author

@virgile-dev thanks for approving the CI test run :)

It failed on the E2E tests now: https://github.com/suitenumerique/docs/actions/runs/17149907792/job/48654751776?pr=1274

  1) [chromium] › __tests__/app-impress/doc-routing.spec.ts:56:7 › Doc Routing › checks 401 on docs/[id] page 

    Error: Timed out 5000ms waiting for expect(locator).toBeVisible()

    Locator: getByText('Log in to access the document')
    Expected: visible
    Received: <element(s) not found>
    Call log:
      - Expect "toBeVisible" with timeout 5000ms
      - waiting for getByText('Log in to access the document')


      87 |     await responsePromise;
      88 |
    > 89 |     await expect(page.getByText('Log in to access the document')).toBeVisible();
         |                                                                   ^
      90 |   });
      91 | });
      92 |
        at /home/runner/work/docs/docs/src/frontend/apps/e2e/__tests__/app-impress/doc-routing.spec.ts:89:67

    Error Context: test-results/app-impress-doc-routing-Do-337d7--checks-401-on-docs-id-page-chromium/error-context.md

    Retry #1 ───────────────────────────────────────────────────────────────────────────────────────

    Error: locator.click: Error: strict mode violation: getByRole('link', { name: '401-doc-parent' }) resolved to 2 elements:
        1) <a class="sc-9ee33ec3-0 gHUMaf" href="/docs/4aaf299c-fe24-40ae-a61f-604753275692/">…</a> aka getByRole('link', { name: 'chromium-2599-0-401-doc-parent' })
        2) <a class="sc-9ee33ec3-0 gHUMaf" href="/docs/0d45dee4-33e4-4650-8c49-a87e7f930e51/">…</a> aka getByRole('link', { name: 'chromium-4480-0-401-doc-parent' })

    Call log:
      - waiting for getByRole('link', { name: '401-doc-parent' })
        - locator resolved to <a class="sc-9ee33ec3-0 bzQxFO" href="/docs/4aaf299c-fe24-40ae-a61f-604753275692/">…</a>
      - attempting click action
        - waiting for element to be visible, enabled and stable
        - element is visible, enabled and stable
        - scrolling into view if needed
        - done scrolling
        - element is not visible
      - retrying click action
        - waiting for element to be visible, enabled and stable
      - element was detached from the DOM, retrying


      83 |     );
      84 |
    > 85 |     await page.getByRole('link', { name: '401-doc-parent' }).click();
         |                                                              ^
      86 |
      87 |     await responsePromise;
      88 |
        at /home/runner/work/docs/docs/src/frontend/apps/e2e/__tests__/app-impress/doc-routing.spec.ts:85:62

    Error Context: test-results/app-impress-doc-routing-Do-337d7--checks-401-on-docs-id-page-chromium-retry1/error-context.md

    attachment #2: trace (application/zip) ─────────────────────────────────────────────────────────
    test-results/app-impress-doc-routing-Do-337d7--checks-401-on-docs-id-page-chromium-retry1/trace.zip
    Usage:

        yarn playwright show-trace test-results/app-impress-doc-routing-Do-337d7--checks-401-on-docs-id-page-chromium-retry1/trace.zip

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

    Retry #2 ───────────────────────────────────────────────────────────────────────────────────────

    Test timeout of 30000ms exceeded.

    Error: locator.click: Test timeout of 30000ms exceeded.
    Call log:
      - waiting for getByRole('link', { name: '401-doc-parent' })
        - locator resolved to <a class="sc-9ee33ec3-0 bzQxFO" href="/docs/a8682550-9c5f-40dd-b770-a19e28cd8326/">…</a>
      - attempting click action
        - waiting for element to be visible, enabled and stable
        - element is visible, enabled and stable
        - scrolling into view if needed
        - done scrolling
      - element was detached from the DOM, retrying


      83 |     );
      84 |
    > 85 |     await page.getByRole('link', { name: '401-doc-parent' }).click();
         |                                                              ^
      86 |
      87 |     await responsePromise;
      88 |
        at /home/runner/work/docs/docs/src/frontend/apps/e2e/__tests__/app-impress/doc-routing.spec.ts:85:62

    Error Context: test-results/app-impress-doc-routing-Do-337d7--checks-401-on-docs-id-page-chromium-retry2/error-context.md

  2) [chromium] › __tests__/app-impress/doc-export.spec.ts:361:7 › Doc Export › it exports the doc with multi columns 

    UnknownErrorException: bad XRef entry

       99 |
      100 |     const pdfBuffer = await cs.toBuffer(await download.createReadStream());
    > 101 |     const pdfData = await pdf(pdfBuffer);
          |                              ^
      102 |
      103 |     expect(pdfData.numpages).toBe(2);
      104 |     expect(pdfData.text).toContain('\n\nHello\n\nWorld'); // This is the doc text
        at UnknownErrorExceptionClosure (/home/runner/work/docs/docs/src/frontend/node_modules/pdf-parse/lib/pdf.js/v1.10.100/build/webpack:/src/shared/util.js:400:37)
        at Object.<anonymous> (/home/runner/work/docs/docs/src/frontend/node_modules/pdf-parse/lib/pdf.js/v1.10.100/build/webpack:/src/shared/util.js:393:30)
        at __w_pdfjs_require__ (/home/runner/work/docs/docs/src/frontend/node_modules/pdf-parse/lib/pdf.js/v1.10.100/build/webpack:/webpack/bootstrap c9be985464785b4e5413:19:1)
        at Object.<anonymous> (/home/runner/work/docs/docs/src/frontend/node_modules/pdf-parse/lib/pdf.js/v1.10.100/build/webpack:/src/pdf.js:24:23)
        at __w_pdfjs_require__ (/home/runner/work/docs/docs/src/frontend/node_modules/pdf-parse/lib/pdf.js/v1.10.100/build/webpack:/webpack/bootstrap c9be985464785b4e5413:19:1)
        at /home/runner/work/docs/docs/src/frontend/node_modules/pdf-parse/lib/pdf.js/v1.10.100/build/webpack:/webpack/bootstrap c9be985464785b4e5413:62:1
        at /home/runner/work/docs/docs/src/frontend/node_modules/pdf-parse/lib/pdf.js/v1.10.100/build/pdf.js:91:10
        at webpackUniversalModuleDefinition (/home/runner/work/docs/docs/src/frontend/node_modules/pdf-parse/lib/pdf.js/v1.10.100/build/webpack:/webpack/universalModuleDefinition:3:1)
        at Object.<anonymous> (/home/runner/work/docs/docs/src/frontend/node_modules/pdf-parse/lib/pdf.js/v1.10.100/build/webpack:/webpack/universalModuleDefinition:10:2)
        at PDF (/home/runner/work/docs/docs/src/frontend/node_modules/pdf-parse/lib/pdf-parse.js:63:29)
        at /home/runner/work/docs/docs/src/frontend/apps/e2e/__tests__/app-impress/doc-export.spec.ts:101:30

    Error Context: test-results/app-impress-doc-export-Doc-b6636--the-doc-with-multi-columns-chromium/error-context.md

  3) [chromium] › __tests__/app-impress/doc-export.spec.ts:426:7 › Doc Export › it injects the correct language attribute into PDF export 

    Test timeout of 30000ms exceeded.

    Error: page.waitForEvent: Test timeout of 30000ms exceeded.
    =========================== logs ===========================
    waiting for event "download"
    ============================================================

      471 |       .click();
      472 |
    > 473 |     const downloadPromise = page.waitForEvent('download', (download) => {
          |                                  ^
      474 |       return download.suggestedFilename().includes(`${randomDocFrench}.pdf`);
      475 |     });
      476 |
        at /home/runner/work/docs/docs/src/frontend/apps/e2e/__tests__/app-impress/doc-export.spec.ts:473:34

    Error Context: test-results/app-impress-doc-export-Doc-39d53-e-attribute-into-PDF-export-chromium/error-context.md

  4) [chromium] › __tests__/app-impress/doc-member-list.spec.ts:11:7 › Document list members › it checks a big list of members 

    Error: Timed out 5000ms waiting for expect(locator).toBeVisible()

    Locator: getByRole('heading', { name: 'Untitled document' })
    Expected: visible
    Received: <element(s) not found>
    Call log:
      - Expect "toBeVisible" with timeout 5000ms
      - waiting for getByRole('heading', { name: 'Untitled document' })


       at app-impress/utils-common.ts:128

      126 |     ).toHaveText(docName);
      127 |   } catch {
    > 128 |     await expect(page.getByRole('heading', { name: docName })).toBeVisible();
          |                                                                ^
      129 |   }
      130 | };
      131 |
        at verifyDocName (/home/runner/work/docs/docs/src/frontend/apps/e2e/__tests__/app-impress/utils-common.ts:128:64)
        at /home/runner/work/docs/docs/src/frontend/apps/e2e/__tests__/app-impress/doc-member-list.spec.ts:13:5

    Error Context: test-results/app-impress-doc-member-lis-439bc-hecks-a-big-list-of-members-chromium/error-context.md

    Retry #1 ───────────────────────────────────────────────────────────────────────────────────────

    Error: Timed out 5000ms waiting for expect(locator).toHaveCount(expected)

    Locator: locator('[data-testid^="doc-share-member-row"]')
    Expected: 20
    Received: 0
    Call log:
      - Expect "toHaveCount" with timeout 5000ms
      - waiting for locator('[data-testid^="doc-share-member-row"]')
        9 × locator resolved to 0 elements
          - unexpected value "0"


      69 |     const loadMore = page.getByTestId('load-more-members');
      70 |
    > 71 |     await expect(elements).toHaveCount(20);
         |                            ^
      72 |
      73 |     await expect(loadMore).toBeHidden();
      74 |   });
        at /home/runner/work/docs/docs/src/frontend/apps/e2e/__tests__/app-impress/doc-member-list.spec.ts:71:28

    Error Context: test-results/app-impress-doc-member-lis-439bc-hecks-a-big-list-of-members-chromium-retry1/error-context.md

    attachment #2: trace (application/zip) ─────────────────────────────────────────────────────────
    test-results/app-impress-doc-member-lis-439bc-hecks-a-big-list-of-members-chromium-retry1/trace.zip
    Usage:

        yarn playwright show-trace test-results/app-impress-doc-member-lis-439bc-hecks-a-big-list-of-members-chromium-retry1/trace.zip

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

  1 failed
    [chromium] › __tests__/app-impress/doc-routing.spec.ts:56:7 › Doc Routing › checks 401 on docs/[id] page 
  3 flaky
    [chromium] › __tests__/app-impress/doc-export.spec.ts:361:7 › Doc Export › it exports the doc with multi columns 
    [chromium] › __tests__/app-impress/doc-export.spec.ts:426:7 › Doc Export › it injects the correct language attribute into PDF export 
    [chromium] › __tests__/app-impress/doc-member-list.spec.ts:11:7 › Document list members › it checks a big list of members 
  121 passed (4.0m)

Not sure I understand the 401 failure here. Do you think that this is related to my change?

@lunika
Copy link
Member

lunika commented Aug 22, 2025

Not sure but I think we have a regression and it is not linked to your PR.
This job is failing on all open PR

@lunika
Copy link
Member

lunika commented Aug 22, 2025

Or maybe it is because your PR is not up to date with the main branch. Try to rebase it

@karlhorky
Copy link
Author

Ok, merged main into the PR now.

@lunika
Copy link
Member

lunika commented Aug 22, 2025

It seems to fix the CI. Can you rebase from main instead of merging the main branch please ? We want to keep a linear history.
Thanks

@karlhorky
Copy link
Author

karlhorky commented Aug 22, 2025

Ok, will do.

Friendly feedback: The process of contributing here as an external contributor is a bit complicated. If some of these rules could be relaxed, in my opinion that would encourage more contribution.

Maybe even possible to achieve things like linear history and commit message formats with automation, to lower the bar for external contributor effort.

The capitalization of the product GitHub has a capital H

Signed-off-by: Karl Horky <[email protected]>
@virgile-dev
Copy link
Collaborator

Thanks for the feedback @karlhorky and for making your changes so quickly
As a government lead project some of these requirements where set by our security (signing commit) and legal department (DCO + Signoff).
The rest is for history readibility and quality in general. If you have automation ideas let us know. We're all hears :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants