-
Notifications
You must be signed in to change notification settings - Fork 398
✨(frontend) use title leading emoji as doc icon in tree #1289
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
Conversation
f437f27
to
d4db4a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is quite nice !
A few comments, plus 2 more things:
-
Do you know how to use Playwright ?
If yes could you write a test e2e about the feature ? I think by updating this test could do the job:
docs/src/frontend/apps/e2e/__tests__/app-impress/doc-header.spec.ts
Lines 55 to 62 in 781c85b
test('it updates the title doc', async ({ page, browserName }) => { await createDoc(page, 'doc-update', browserName, 1); const docTitle = page.getByRole('textbox', { name: 'doc title input' }); await expect(docTitle).toBeVisible(); await docTitle.fill('Hello World'); await docTitle.blur(); await verifyDocName(page, 'Hello World'); }); -
I can see you are merging
main
in the branch, could you rebase it instead as we prefer a linear history? Thanks!

src/frontend/apps/impress/src/features/docs/doc-management/components/DocIcon.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-management/components/DocIcon.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-management/utils.ts
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-management/__tests__/utils.test.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-management/components/SimpleDocItem.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/docs/doc-management/components/SimpleDocItem.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is not managed, if the document is pinned the emoji will not display.
We could eventually create a svg with only the "pin" icon and add it on the top right, wdyt ?
docs/src/frontend/apps/impress/src/features/docs/doc-management/components/SimpleDocItem.tsx
Lines 62 to 67 in 1c6c58a
{isPinned ? ( | |
<PinnedDocumentIcon | |
aria-hidden="true" | |
aria-label={t('Pin document icon')} | |
color={colorsTokens['primary-500']} | |
/> |

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AntoLC,
Thanks for the thorough review !
Indeed there is something off with the pinned docs.
Do you mean something like this (with a proper pin icon of course)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking more about something like that ?
@rl-83 @virgile-dev WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR to handle this case proposed here : #1358
069cba4
to
fb454bb
Compare
Hi @AntoLC, |
1040b9e
to
8770ab5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About this commit (📝(frontend) propose a condensed version of the frontend README), I think we should do it in an other PR, it is a bit out of scope, it will be easier for review.
About this commit (✨(frontend) leading emoji in title, PR feedback applied), can it be a fixup commit instead, because its only purpose is fixing this commit ✨(frontend) use title first emoji as doc icon in tree.
8770ab5
to
e92c7a7
Compare
Removed. Moved here #1318
|
Yes and it's ok. Once the review process done you will squash all your fixup commits : https://jordanelver.co.uk/blog/2020/06/04/fixing-commits-with-git-commit-fixup-and-git-rebase-autosquash/ |
Then should this |
I would say no to prevent merging fixup commits. This is not what we want. They can live in a PR but not in the main branch. |
But the part of the CI checking is fixup commits are present should probably move after the gitlint check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I improve a bit the e2e test.
✅ All good, let's squash the fixup, then merge !
6759ebc
to
72ce96d
Compare
Implemented emoji detection system, new DocIcon component.
It was failing at 20min, increase the timeout to 30 min
7f1e3f6
to
75f2e54
Compare
Purpose
This pull request intent to fix this issue.
If the first unicode char of a title is an emoji, this emoji will replace the standard icon in the tree.
Proposal
I have made use of the library emoji-regex, because I didn't find a solution simple enough with a regex which would be compatible with ES5.
I took the liberty to add a
make frontend-test
command.E2E Frontend test are failing, not sure it relates to this PR. I also had to increase the GitHub Action timeout so that it complete.
External contributions
Please ensure the following items are checked before submitting your pull request:
git commit --signoff
(DCO compliance)git commit -S
)<gitmoji>(type) title description
## [Unreleased]
section (if noticeable change)