fix hyphen issue for sync and controller code #1725
Conversation
There was a problem hiding this comment.
Great work @yadvirkaur ji 🔥. Can you please resolve the below comments and also please add a unit test for this function and/or component.
And one more request please whenever you work/touch a file lets convert it to Typescript immediately to get better type support and enhance DX.
https://github.com/KhalisFoundation/sttm-web/blob/dev/CONTRIBUTING.md
| @@ -274,26 +273,18 @@ export default class WebControllerPage extends React.PureComponent { | |||
| pattern="[A-Z,a-z]{3}-[A-Z,a-z]{3}" | |||
| onKeyUp={e => { | |||
| const typedValue = e.currentTarget.value; | |||
| if (isMobile) { | |||
There was a problem hiding this comment.
@yadvirkaur I can see there is difference in the logic in Mobile and Desktop. Is it something reconsidered now?
There was a problem hiding this comment.
Previously:
For the Bani Controller:
-
Mobile Devices: The code checked if the user had typed four characters without a hyphen and then inserted a hyphen after the third character.
-
Other Devices: The code added a hyphen automatically after the third character.
For Sangat Sync:
The code for all devices added a hyphen automatically after the third character.
Now:
I've now unified the logic for the Bani Controller to ensure consistent behaviour across all devices. The code now adds a hyphen automatically after the third character for all devices and ensures that no extra hyphens are added.
There was a problem hiding this comment.
@yadvirkaur cool, thanks for the explainations, I totally got it. It's just I am unaware of these changes, if these are the changes requested lets do it. Also I would add, lets please make sure all these changes are mentioned in the ticket too, easier to review and followup.
Otherwise I would request to kindly confirm, as I am sure there might be good reason for this seperation.
There was a problem hiding this comment.
@gurjit03 hnji veerji!
The mobile-specific logic for the Bani controller is fine as it is since it adds a hyphen after 4 characters only if the user hasn't already added one, so no risk of double hyphens there.
However, this can cause confusion between the Bani Controller and Sangat Sync since they handle hyphens differently for mobile devices.
@saintsoldierx @Gauravjeetsingh veerji please confirm if we need separation of logic or if we can have a unified logic.
There was a problem hiding this comment.
I don't remember the reason for separate logic, but if its working with the unified logic, let's keep that. Realistically it makes sense to have same logic for both mobile and desktop browsers.
src/js/pages/WebController/index.js
Outdated
| if (isParsedValueExist && typedChar !== 'Backspace') { | ||
| e.currentTarget.value = typedValue + '-'; | ||
| } else if (typedChar === '-' && hasHyphen) { | ||
| e.currentTarget.value = typedValue.replace(/-+/g, '-'); |
There was a problem hiding this comment.
@yadvirkaur I might be wrong here but we are replacing /-+/g both - and + aint? Or does it represents - n number of times? if so, can you please update implementation a bit to be more clear. It can cause confusion.
There was a problem hiding this comment.
The expression (/-+/g, '-') here matches one or more hyphens and replaces them with a single hyphen. It's not related to the + character itself. To make it clearer, I can update it to (/[-]+/g, '-'). Let me know if that works!
There was a problem hiding this comment.
@yadvirkaur yes that works perfectly fine
npm test& fixed newly introduced lint errors.#1111