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

(PC-32770) fix(highlightLinks): fix links highlighting #7152

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tconte-pass
Copy link
Contributor

Link to JIRA ticket: https://passculture.atlassian.net/browse/PC-32770

Flakiness

If I had to re-run tests in the CI due to flakiness, I add the incident on Notion

Checklist

I have:

  • Made sure my feature is working on web.
  • Made sure my feature is working on mobile (depending on relevance : real or virtual devices)
  • Written unit tests native (and web when implementation is different) for my feature.
  • Added a screenshot for UI tickets or deleted the screenshot section if no UI change
  • If my PR is a bugfix, I add the link of the "résolution de problème sur le bug" on Notion

Screenshots

delete if no UI change

Platform Mockup/Before After
iOS
Android
Phone - Chrome
Desktop - Chrome

Copy link

github-actions bot commented Nov 6, 2024

Performance Comparison Report

Significant Changes To Render Duration

There are no entries

Meaningless Changes To Render Duration

Show entries
Name Render Duration Render Count
Performance test for EndedBookings page 33.5 ms → 35.0 ms (+1.5 ms, +4.5%) 7 → 7
Performance test for Profile page 6.2 ms → 7.0 ms (+0.8 ms, +12.9%) 5 → 5
Performance test for Venue page 2.6 ms → 2.9 ms (+0.3 ms, +11.5%) 2 → 2
Search Landing Page - Performance test for Search Landing page 14.6 ms → 14.8 ms (+0.2 ms, +1.4%) 5 → 5
Search Results - Performance test for Search Results page 14.3 ms → 14.3 ms 5 → 5
Performance test for Favorites page 66.8 ms → 65.8 ms (-1.0 ms, -1.5%) 6 → 6
Performance test for Offer page 74.1 ms → 72.0 ms (-2.1 ms, -2.8%) 2 → 2
Performance test for Bookings page 104.2 ms → 101.8 ms (-2.4 ms, -2.3%) 11 → 11
Show details
Name Render Duration Render Count
Performance test for EndedBookings page Baseline
Mean: 33.5 ms
Stdev: 3.0 ms (9.0%)
Runs: 37 37 36 35 34 33 32 32 32 27

Current
Mean: 35.0 ms
Stdev: 1.6 ms (4.7%)
Runs: 38 36 36 36 35 35 34 34 34 32
Baseline
Mean: 7
Stdev: 0 (0.0%)
Runs: 7 7 7 7 7 7 7 7 7 7

Current
Mean: 7
Stdev: 0 (0.0%)
Runs: 7 7 7 7 7 7 7 7 7 7
Performance test for Profile page Baseline
Mean: 6.2 ms
Stdev: 0.6 ms (10.2%)
Runs: 7 7 7 6 6 6 6 6 6 5

Current
Mean: 7.0 ms
Stdev: 1.2 ms (16.5%)
Runs: 9 8 8 8 7 6 6 6 6 6
Baseline
Mean: 5
Stdev: 0 (0.0%)
Runs: 5 5 5 5 5 5 5 5 5 5

Current
Mean: 5
Stdev: 0 (0.0%)
Runs: 5 5 5 5 5 5 5 5 5 5
Performance test for Venue page Baseline
Mean: 2.6 ms
Stdev: 0.7 ms (26.9%)
Runs: 4 3 3 3 3 2 2 2 2 2

Current
Mean: 2.9 ms
Stdev: 0.9 ms (30.2%)
Runs: 4 4 4 3 3 3 2 2 2 2
Baseline
Mean: 2
Stdev: 0 (0.0%)
Runs: 2 2 2 2 2 2 2 2 2 2

Current
Mean: 2
Stdev: 0 (0.0%)
Runs: 2 2 2 2 2 2 2 2 2 2
Search Landing Page - Performance test for Search Landing page Baseline
Mean: 14.6 ms
Stdev: 1.4 ms (9.8%)
Runs: 17 16 15 15 15 15 14 14 13 12

Current
Mean: 14.8 ms
Stdev: 1.3 ms (8.9%)
Runs: 17 16 16 15 15 15 14 14 13 13
Baseline
Mean: 5
Stdev: 0 (0.0%)
Runs: 5 5 5 5 5 5 5 5 5 5

Current
Mean: 5
Stdev: 0 (0.0%)
Runs: 5 5 5 5 5 5 5 5 5 5
Search Results - Performance test for Search Results page Baseline
Mean: 14.3 ms
Stdev: 0.8 ms (5.8%)
Runs: 15 15 15 15 15 14 14 14 13 13

Current
Mean: 14.3 ms
Stdev: 1.1 ms (7.4%)
Runs: 16 15 15 15 15 14 14 13 13 13
Baseline
Mean: 5
Stdev: 0 (0.0%)
Runs: 5 5 5 5 5 5 5 5 5 5

Current
Mean: 5
Stdev: 0 (0.0%)
Runs: 5 5 5 5 5 5 5 5 5 5
Performance test for Favorites page Baseline
Mean: 66.8 ms
Stdev: 2.3 ms (3.5%)
Runs: 71 69 69 67 67 67 65 65 64 64

Current
Mean: 65.8 ms
Stdev: 2.0 ms (3.0%)
Runs: 68 68 68 67 66 66 65 64 63 63
Baseline
Mean: 6
Stdev: 0 (0.0%)
Runs: 6 6 6 6 6 6 6 6 6 6

Current
Mean: 6
Stdev: 0 (0.0%)
Runs: 6 6 6 6 6 6 6 6 6 6
Performance test for Offer page Baseline
Mean: 74.1 ms
Stdev: 3.2 ms (4.3%)
Runs: 80 79 75 75 74 72 72 72 71 71

Current
Mean: 72.0 ms
Stdev: 2.3 ms (3.2%)
Runs: 77 74 73 72 72 72 71 70 70 69
Baseline
Mean: 2
Stdev: 0 (0.0%)
Runs: 2 2 2 2 2 2 2 2 2 2

Current
Mean: 2
Stdev: 0 (0.0%)
Runs: 2 2 2 2 2 2 2 2 2 2
Performance test for Bookings page Baseline
Mean: 104.2 ms
Stdev: 3.2 ms (3.1%)
Runs: 110 108 107 105 104 103 102 101 101 101

Current
Mean: 101.8 ms
Stdev: 4.0 ms (3.9%)
Runs: 108 104 104 104 103 102 101 100 99 93
Baseline
Mean: 11
Stdev: 0 (0.0%)
Runs: 11 11 11 11 11 11 11 11 11 11

Current
Mean: 11
Stdev: 0 (0.0%)
Runs: 11 11 11 11 11 11 11 11 11 11

Changes To Render Count

There are no entries

Added Scenarios

There are no entries

Removed Scenarios

There are no entries

Generated by 🚫 dangerJS against 23dbc1a

@tconte-pass tconte-pass force-pushed the pc-32770-fix-links-highlighting branch from 589efbf to a4312e1 Compare November 6, 2024 13:40
The purpose is to exclude cases where a dot is not followed by a space
like 'final word.Then'.
We'll continue to make the mistake on inclusive writing for now,
like 'revigorant.e'.
@tconte-pass tconte-pass force-pushed the pc-32770-fix-links-highlighting branch from a4312e1 to aaa6721 Compare November 6, 2024 13:46
Copy link

sonarcloud bot commented Nov 6, 2024

Copy link

sonarcloud bot commented Nov 6, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
71.4% Coverage on New Code (required ≥ 85%)

See analysis details on SonarCloud

@@ -11,6 +11,10 @@ const description1WithoutUrl = `PRESSE / ILS EN PARLENT !
« Hilarant » Vanity Fair
Voir plus de critiques en suivant le lien suivant :`

const descriptionWithoutSpaceAfterDot = `Lorem ipsum dolor sit amet consectetur adipiscing elit.Sed non risus.`

const descriptionWitUppercaseLink = `Lorem ipsum dolor sit amet consectetur adipiscing Elit.sed non risus.`
Copy link
Contributor

Choose a reason for hiding this comment

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

descriptionWithUppercaseLink ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Un lien qui commence par une majuscule ... Tu aurais nommé ça comment ?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah oui mon comm est pas claire , je voulais dire qu'il manquait le 'H' de 'with' :)

@@ -11,6 +11,10 @@ const description1WithoutUrl = `PRESSE / ILS EN PARLENT !
« Hilarant » Vanity Fair
Voir plus de critiques en suivant le lien suivant :`

const descriptionWithoutSpaceAfterDot = `Lorem ipsum dolor sit amet consectetur adipiscing elit.Sed non risus.`

const descriptionWitUppercaseLink = `Lorem ipsum dolor sit amet consectetur adipiscing Elit.sed non risus.`
Copy link
Contributor

Choose a reason for hiding this comment

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

Je comprend pas où est le lien, peut être qu'un veritable exemple plutôt dque du Lorem ipsum serait plus parlant ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Elit.sed est considéré comme un lien

@@ -177,7 +174,7 @@ describe('highlightLinks', () => {
)

expect(parsedDescription).toEqual([
<ExternalLink key="external-link-0" url="http://www.penofchaos.com/warham/donjon.htm" />,
<ExternalLink key="external-link-0" url="www.penofchaos.com/warham/donjon.htm" />,
Copy link
Contributor

Choose a reason for hiding this comment

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

ce type de lien fonctionne ? (web, iOS, Android)

@@ -7,20 +7,18 @@ type ParsedDescription = Array<string | React.ReactNode>

const externalNavRegex = new RegExp(
/((^|\s)|https?:\/\/)[a-z]([-a-z0-9:%._+~#=]*[a-z0-9])?\.[a-z0-9]{1,6}([/?#]\S*)?(\s|$)/,
Copy link
Contributor

Choose a reason for hiding this comment

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

il me semble que coté pro, iels ont changés la façon de gérer les liens dans les descriptions

est-on encore aligné ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pourquoi est-ce qu'on doit être alignés entre leur façon de récupérer/afficher une URL et la nôtre ?

Copy link
Contributor

Choose a reason for hiding this comment

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

si je suis un pro, que je prends le temps d'écrire une description qui est super bien, d'ajouter des liens où il faut et que :

  • coté jeunes, des liens s'affichent à des endroits qui n'y étaient pas coté pro
  • coté jeunes, des liens ne s'affichent pas à des endroits qui y étaient coté pro

je serais mécontent, et je dirais que c'est buggué

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