fix(tp/jobseeker): add validation for year inputs in experience and education

[katamatata]

What Github issue does this PR relate to? Insert link.

#381

What should the reviewer know?

In this PR, I also edited validation messages to be more consistent and polite and deleted those not required according to this ticket.

[katamatata]

The wording of validation messages was approved by @miriam-aha.

[ericbolikowski]

Pretty good work here, @katamatata. I did a cursory review and a couple things come to mind; let's discuss more in our session tomorrow morning.

[ericbolikowski]

@katamatata and I talked about this PR today. Outcomes:

• Convert the year input from type="number" to .... nothing (to make it a string)
• Use yup (or another means like parseInt) to cast (as well as validate) the year input string into a number

[ericbolikowski]

@helloanil, @miriam-aha: Kate and I have a question about the close button in the modal. Can you both please see this Loom video and share your thoughts? Thanks!

https://www.loom.com/share/26fa447ff2d64e21b5a19e4f665ce15e

[helloanil]

@helloanil, @miriam-aha: Kate and I have a question about the close button in the modal. Can you both please see this Loom video and share your thoughts? Thanks!

https://www.loom.com/share/26fa447ff2d64e21b5a19e4f665ce15e

I loved the cheerful loom @ericbolikowski and @katamatata. I have several comments here:

• Speaking of the close icon, I'd definitely prefer the way Connect handles it. Inside the popup, at the right top corner.
• Speaking of the circle around the icon, there should be a padding for sure, great catch!
• Another thing to mention is, how popup designs are very different in both places. That's probably the root cause of this issue. (Notice the size of the popup title, the divider line under the title, the border-radius of the dialog and so on). Probably having a single popup design that would work for both platforms would be the absolute best solution to this.

However, since the third point requires a bit of collaboration with the design team, we can definitely start small by fixing the close icon, and then see how much effort is needed to unify the popup designs, using the best pieces from two designs we have atm.

Let me know what you think!

[miriam-aha]

Thanks @katamatata and @ericbolikowski for catching this!

I agree with @helloanil here on using the Connect close button for all popup windows. I think the circle does not need to be bigger, but changing it to a darker color (dark grey?) could have the same effect of highlighting it.

Also, I agree we should ultimately align the two popup designs. I will keep that in mind to discuss with the design team once a few of the more pressing issues are out of the way. Thanks all!! 🙌

[ericbolikowski]

Thanks @miriam-aha and @helloanil for the swift answers!

@miriam-aha, how about we then create two tickets? One for the coding work we can do before we talk to designers (perhaps @katamatata wants to take that on?). And one for the stuff we need design input on.

[katamatata]

My five cents to Anil's third point :)
IMO the popups on TP have a slightly different purpose. They offer the user more context about how to fill up the form. The popups on CON are pretty straightforward (see attached) - no need for context there. Thus the single popup design might be hard to achieve, but there is indeed an area for improvement for either of two.

[katamatata]

@miriam-aha, how about we then create two tickets? One for the coding work we can do before we talk to designers (perhaps @katamatata wants to take that on?). And one for the stuff we need design input on.

@ericbolikowski, ticket #388 in this PR is about the issue with the close icon. I could update it and delete it from this PR. Is that what you meant?

[miriam-aha]

Thanks for that observation @katamatata. I think what could be aligned is the divider under the title and also the font size of the title. Your suggestion to update the existing ticket #388 sounds great. @ericbolikowski Are you okay with that?

[ericbolikowski]

@miriam-aha @katamatata yes, sounds good: let's update #388 to reflect all the changes we want do to in the very near future: replicate how the icon is used from CON to TP; on hover show darker grey and greater padding.

After that's done we can see if further design input is needed.

When it comes to uniform title font size and divider under title, I would consult the design team first, meaning a separate Build UI/UX issue. The header area of CON modals contain a simple title, whereas in TP there's kind of one all-uppercase main title + one mixed-case subtitle (with different font sizes). So it should be carefully reviewed to ensure good look n' feel.

[miriam-aha]

Hi @ericbolikowski I spoke with @katamatata yesterday and we agreed to consult the design team in any case since we found even more different designs for the close button. So before we add any new color or padding we want to make sure this is aligned with what we have already. Once I have more info I will let you know.

[ericbolikowski]

Oh wow 😁 Happy this triggered some clean-up then, making things more uniform

[semgrep-app]

Semgrep found 4 react-no-refs findings:

• apps/redi-talent-pool/src/components/organisms/jobseeker-profile-editables/EditableEducation.tsx: L304-404, L296-409
• apps/redi-talent-pool/src/components/organisms/jobseeker-profile-editables/EditableProfessionalExperience.tsx: L299-397, L291-402

ref usage found. refs give direct DOM access and may create a possibility for XSS, which could cause sensitive information such as user cookies to be retrieved by an attacker. Instead, avoid direct DOM manipulation or use DOMPurify to sanitize HTML before writing it into the page.

_{⚪️ This finding does not block your pull request.
🙈 From typescript.react.security.audit.react-no-refs.react-no-refs.}

[semgrep-app]

Semgrep found 4 react-html-element-spreading findings:

• apps/redi-talent-pool/src/components/organisms/jobseeker-profile-editables/EditableProfessionalExperience.tsx: L287-396, L295-391
• apps/redi-talent-pool/src/components/organisms/jobseeker-profile-editables/EditableEducation.tsx: L292-403, L300-398

It is a good practice to avoid spreading for JSX attributes. This prevents accidentally passing dangerouslySetInnerHTML to an element.

_{⚪️ This finding does not block your pull request.
🙈 From typescript.react.security.audit.react-html-element-spreading.react-html-element-spreading.}

[katamatata]

• Convert the year input from type="number" to .... nothing (to make it a string)
• Use yup (or another means like parseInt) to cast (as well as validate) the year input string into a number

@ericbolikowski Done and ready for review.

[ericbolikowski]

LGTM