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

chore: [PE-737] CGN capitalize text apostrophe case #6293

Merged
merged 15 commits into from
Oct 17, 2024

Conversation

LeleDallas
Copy link
Contributor

Short description

This PR updates the capitalize function to correctly handle words with apostrophes, ensuring that words like Dall'Ara remain Dall'Ara and not Dall'ara.

List of changes proposed in this pull request

  • Added new test cases to ensure the function works as expected with apostrophes.
  • Updated the capitalize function to correctly handle words with apostrophes.
  • Used RegExp.exec() to match and preserve leading and trailing spaces.

How to test

  1. Ensure your development environment is set up and the project is running.
  2. Run the test suite to verify the changes
  3. Check that all tests pass, including the new test cases for words with apostrophes and leading/trailing spaces.

Or

  1. Modify the CgnOwnershipInformation file with a hardcoded word containing an apostrophe.
  2. Verify that the CgnOwnershipInformation component correctly capitalizes words with apostrophes when rendered.

##Preview
Screenshot 2024-10-17 at 09 41 56

test suite to ensure that capitalize function correctly handles words with apostrophes
@LeleDallas LeleDallas requested a review from Hantex9 October 17, 2024 07:43
@LeleDallas LeleDallas requested a review from a team as a code owner October 17, 2024 07:43
@pagopa-github-bot pagopa-github-bot changed the title fix: [PE-737] CGN capitalize text apostrophe case feat: [PE-737] CGN capitalize text apostrophe case Oct 17, 2024
@pagopa-github-bot
Copy link
Collaborator

pagopa-github-bot commented Oct 17, 2024

Affected stories

  • ⚙️ PE-737: [FE] gestire nuova casistica capitalize nome

Generated by 🚫 dangerJS against 0254968

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 46.98%. Comparing base (4f204b4) to head (0254968).
Report is 616 commits behind head on master.

Files with missing lines Patch % Lines
ts/features/bonus/cgn/screens/CgnDetailScreen.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6293      +/-   ##
==========================================
- Coverage   48.42%   46.98%   -1.45%     
==========================================
  Files        1488     1802     +314     
  Lines       31617    36527    +4910     
  Branches     7669     8759    +1090     
==========================================
+ Hits        15311    17161    +1850     
- Misses      16238    19301    +3063     
+ Partials       68       65       -3     
Files with missing lines Coverage Δ
.../cgn/components/detail/CgnOwnershipInformation.tsx 20.00% <ø> (-37.15%) ⬇️
ts/utils/strings.ts 55.22% <100.00%> (+4.31%) ⬆️
ts/features/bonus/cgn/screens/CgnDetailScreen.tsx 4.54% <0.00%> (-9.41%) ⬇️

... and 1414 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d15d93f...0254968. Read the comment docs.

Copy link
Contributor

@Hantex9 Hantex9 left a comment

Choose a reason for hiding this comment

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

Great job on this PR! I've left a couple of minor suggestions

ts/utils/__tests__/string.test.ts Outdated Show resolved Hide resolved
ts/utils/strings.ts Outdated Show resolved Hide resolved
@freddi301
Copy link
Collaborator

would you consider a simpler algorithm like

function capitalizePersonName(text: string) {
  // find all indices of characters that are preceded by a space or apostrophe
  const indices = Array.from(text.matchAll(/(?<=\s|')\w/g)).map(match => match.index);
  // uppercase each character at the found indices, lowercase others
  return text.split('').map((char, index) => indices.includes(index) || index === 0 ? char.toLocaleUpperCase() : char.toLocaleLowerCase()).join('');
}

console.log(capitalizePersonName("frederik"))
console.log(capitalizePersonName("FREDERIK"))
console.log(capitalizePersonName("frederik van der linde"))
console.log(capitalizePersonName("FREDERIK van der LINDE"))
console.log(capitalizePersonName("o'connor"))
console.log(capitalizePersonName("O'CONNOR"))

@LeleDallas
Copy link
Contributor Author

would you consider a simpler algorithm like

function capitalizePersonName(text: string) {
  // find all indices of characters that are preceded by a space or apostrophe
  const indices = Array.from(text.matchAll(/(?<=\s|')\w/g)).map(match => match.index);
  // uppercase each character at the found indices, lowercase others
  return text.split('').map((char, index) => indices.includes(index) || index === 0 ? char.toLocaleUpperCase() : char.toLocaleLowerCase()).join('');
}

console.log(capitalizePersonName("frederik"))
console.log(capitalizePersonName("FREDERIK"))
console.log(capitalizePersonName("frederik van der linde"))
console.log(capitalizePersonName("FREDERIK van der LINDE"))
console.log(capitalizePersonName("o'connor"))
console.log(capitalizePersonName("O'CONNOR"))

Thank you for your feedback!
I believe that approach might be more complex and harder to maintain compared to our current solution

@LeleDallas LeleDallas requested a review from Hantex9 October 17, 2024 09:18
Hantex9
Hantex9 previously approved these changes Oct 17, 2024
Copy link
Contributor

@Hantex9 Hantex9 left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@freddi301
Copy link
Collaborator

freddi301 commented Oct 17, 2024

From what I understood from other conversations, the user name and surname are provided by external systems and we are meant to disregard any capitalization coming from there and apply ours.
The test should be

it("should return a string where each word has the first char in uppercase even after an apostrophe", () => {
   expect(capitalize("Capit'Alize")).toEqual("Capit'Alize");
 });

 it("should return a string where each word has the first char in uppercase even after an apostrophe-2", () => {
   expect(capitalize("capit'alize")).toEqual("Capit'Alize");
 });

 it("should return a string where each word has the first char in uppercase even after an apostrophe-3", () => {
   expect(capitalize("Capit'alize")).toEqual("Capit'Alize");
 });

I do not remember the collegue who said this tough, and that if he actually meant this, should we verify? (@Hantex9 can you think about someone who could said this?)

@Hantex9
Copy link
Contributor

Hantex9 commented Oct 17, 2024

From what I understood from other conversations, the user name and surname are provided by external systems and we are meant to disregard any capitalization coming from there and apply ours. The test should be

it("should return a string where each word has the first char in uppercase even after an apostrophe", () => {
   expect(capitalize("Capit'Alize")).toEqual("Capit'Alize");
 });

 it("should return a string where each word has the first char in uppercase even after an apostrophe-2", () => {
   expect(capitalize("capit'alize")).toEqual("Capit'Alize");
 });

 it("should return a string where each word has the first char in uppercase even after an apostrophe-3", () => {
   expect(capitalize("Capit'alize")).toEqual("Capit'Alize");
 });

I do not remember the collegue who said this tough, and that if he actually meant this, should we verify? (@Hantex9 can you think about someone who could said this?)

I'm not entirely sure about this, as not all surnames have a capital letter following an apostrophe. Maybe @michaeldisaro could provide us with more details on this.

@freddi301
Copy link
Collaborator

How these examples should be capitalized?

D'AMICO
D' AMICO
d'amico
d'amico

@LeleDallas
Copy link
Contributor Author

How these examples should be capitalized?

D'AMICO D' AMICO d'amico d'amico

  • D'AMICO
  • D' AMICO
  • D'amico (string are the same for last 2 points)
  • For d' amico it will be D' Amico

@Hantex9
Copy link
Contributor

Hantex9 commented Oct 17, 2024

How these examples should be capitalized?

D'AMICO D' AMICO d'amico d'amico

For us, the important part is ensuring that the initials are uppercase. Beyond that, the capitalization that follows can't be something we can reliably predict. On our end, how can we be certain that if we're provided with D'AMICO, it should be capitalized as D'Amico and not D'amico?

@thisisjp what are your thoughts from a product & design perspective?

@LeleDallas
Copy link
Contributor Author

How these examples should be capitalized?
D'AMICO D' AMICO d'amico d'amico

For us, the important part is ensuring that the initials are uppercase. Beyond that, the capitalization that follows can't be something we can reliably predict. On our end, how can we be certain that if we're provided with D'AMICO, it should be capitalized as D'Amico and not D'amico?

@thisisjp what are your thoughts from a product & design perspective?

So, do you think we should first convert the entire string to lowercase and then capitalize the first character, as well as the first character after an apostrophe?

The function now capitalize only the initials (as before), including the letter after the apostrophe.
@LeleDallas LeleDallas requested a review from Hantex9 October 17, 2024 13:09
@pagopa-github-bot pagopa-github-bot changed the title feat: [PE-737] CGN capitalize text apostrophe case chore: [PE-737] CGN capitalize text apostrophe case Oct 17, 2024
ts/utils/strings.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Hantex9 Hantex9 left a comment

Choose a reason for hiding this comment

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

LGTM!

@LeleDallas LeleDallas merged commit 5552357 into master Oct 17, 2024
13 checks passed
@LeleDallas LeleDallas deleted the PE-737-cgn-capitalize-text-apostrophe-case branch October 17, 2024 16:14
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.

4 participants