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.

Sign up for GitHub

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

Jump to bottom

#10289: Cursor position is shifted incorrectly in Localized number field component #10312

Merged
merged 10 commits into from
May 20, 2024
Merged

#10289: Cursor position is shifted incorrectly in Localized number field component #10312

merged 10 commits into from
May 20, 2024

Conversation

mahmoudadel54
Copy link
Collaborator

Description

In this PR, fixing cursor position shifting incorrectly in Localized number field component

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

Issue

#10289

What is the current behavior?
#10289

What is the new behavior?
For Localized number field component, if user edits/adds/deletes numbers into the input, the cursor will move in a correct way.

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • No

Other useful information

@mahmoudadel54
geosolutions-it#10289: Cursor position is shifted incorrectly in Loca…
edae9e7 
…lized number field component

Description:
- fix the jumping cursor to unexpected positions for localized numeric input
@mahmoudadel54
geosolutions-it#10289: Cursor position is shifted incorrectly in Loca…
3a5f86a 
…lized number field component

Description:
- replace logic in updateDidMount to onKeyUp
- remove unused comments
@mahmoudadel54
geosolutions-it#10289: Cursor position is shifted incorrectly in Loca…
056a766 
…lized number field component

Description:
- remove unused method in IntlNumberFormControl
@mahmoudadel54 mahmoudadel54 added this to the 2024.01.01 milestone May 13, 2024
@mahmoudadel54 mahmoudadel54 self-assigned this May 13, 2024
@mahmoudadel54
geosolutions-it#10136: Search for Map CRS coordinates
9aa5337 
Description:
- resolve FE failure in unit test
@mahmoudadel54 mahmoudadel54 linked an issue May 14, 2024 that may be closed by this pull request
Cursor position is shifted incorrectly in Localized number field component #10289
Closed
1 task
dsuren1
dsuren1 requested changes May 15, 2024
View reviewed changes
Copy link
Contributor

@dsuren1 dsuren1 left a comment

Choose a reason for hiding this comment

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

@mahmoudadel54
In most cases, the cursor position behaves correctly, except for few these. Kindly check. Thanks!

  • With fractional/decimal digit, on focus field value becomes unformatted

    fractional_unformatted.mp4

  • Follow up to above issue, the cursor position is incorrect when deleting fractional digits probably caused by unformatted value

    unformatted_cursor_position_incorrect.mp4

web/client/components/I18N/IntlNumberFormControl.jsx Outdated Show resolved Hide resolved
web/client/components/I18N/IntlNumberFormControl.jsx Outdated Show resolved Hide resolved
web/client/components/I18N/IntlNumberFormControl.jsx Outdated Show resolved Hide resolved
@mahmoudadel54
Copy link
Collaborator Author

mahmoudadel54 commented May 15, 2024 •
edited

@mahmoudadel54 In most cases, the cursor position behaves correctly, except for few these. Kindly check. Thanks!

  • With fractional/decimal digit, on focus field value becomes unformatted

    fractional_unformatted.mp4

  • Follow up to above issue, the cursor position is incorrect when deleting fractional digits probably caused by unformatted value

    unformatted_cursor_position_incorrect.mp4

These are because this regex which located in NumericInput:

MapStore2/web/client/libs/numeric-input/NumericInput.jsx

Line 11 in 4c314d5

const RE_INCOMPLETE_NUMBER = /^([+-]|[+-]?\d+\.\d+0*|[+-]\.0*|[+-]?\d+\.)?$/;

it sees number like: 321.235 as incomplete number

@dsuren1 I need more clarifications about the cases this regular expression should handle.

@mahmoudadel54
geosolutions-it#10289: Cursor position is shifted incorrectly in Loca…
89d16ad 
…lized number field component [resolve review comments]

Description:
- fix issue of on focus effect in NumericInput
@mahmoudadel54
geosolutions-it#10289: Cursor position is shifted incorrectly in Loca…
c47ff1c 
…lized number field component [resolve review comments]

Description:
- fix FE issue in onKeyDown in IntlNumberFormControl
@mahmoudadel54
geosolutions-it#10289: Cursor position is shifted incorrectly in Loca…
57e4c4e 
…lized number field component [resolve review comments]

Description:
- revert unnecessary change
dsuren1
dsuren1 requested changes May 16, 2024
View reviewed changes
Copy link
Contributor

@dsuren1 dsuren1 left a comment •
edited

Choose a reason for hiding this comment

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

@mahmoudadel54

  • Add unit tests for these cases

  • Cursor position shift incorrectly as we type. I didn't see this before previous commit. Kindly check it. Thanks!

    incorrect_cursor.mp4

@mahmoudadel54
Copy link
Collaborator Author

@mahmoudadel54

  • Add unit tests for these cases

  • Cursor position shift incorrectly as we type. I didn't see this before previous commit. Kindly check it. Thanks!

    incorrect_cursor.mp4

Hi @dsuren1
can you please double check again? as I made double check and it works well

cursor.issue.working.as.expected.mp4

@dsuren1
Copy link
Contributor

dsuren1 commented May 16, 2024 •
edited

Hi @dsuren1 can you please double check again? as I made double check and it works well

Like I mentioned over chat, it happens when user types the value a little fast. Kindly give it a try like we discussed. Thanks!

@mahmoudadel54
geosolutions-it#10289: Cursor position is shifted incorrectly in Loca…
0c8a864 
…lized number field component [resolve review comments]

Description:
- move the fixing logic of cursor position to componentDidUpdate instead of onKeyUp
dsuren1
dsuren1 requested changes May 17, 2024
View reviewed changes
Copy link
Contributor

@dsuren1 dsuren1 left a comment

Choose a reason for hiding this comment

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

@mahmoudadel54
Before reviewing this new change, I would also like this component to support onKeyUp function from consuming component in tandem with the changes incorporated for handing cursor

@mahmoudadel54
geosolutions-it#10289: Cursor position is shifted incorrectly in Loca…
bad9be8 
…lized number field component [resolve review comments]

Description:
- add onKeyUp as a prop into IntlNumberFormControl for any expected future consuming from a parent comp
@mahmoudadel54
Copy link
Collaborator Author

@mahmoudadel54 Before reviewing this new change, I would also like this component to support onKeyUp function from consuming component in tandem with the changes incorporated for handing cursor

Done

dsuren1
dsuren1 requested changes May 17, 2024
View reviewed changes
Copy link
Contributor

@dsuren1 dsuren1 left a comment

Choose a reason for hiding this comment

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

@mahmoudadel54
Rest looks good. Just minor amendments. And still unit tests are missing

web/client/components/I18N/IntlNumberFormControl.jsx Outdated Show resolved Hide resolved
web/client/components/I18N/IntlNumberFormControl.jsx Outdated Show resolved Hide resolved
web/client/components/I18N/IntlNumberFormControl.jsx Outdated Show resolved Hide resolved
@mahmoudadel54
geosolutions-it#10289: Cursor position is shifted incorrectly in Loca…
e5e1c53 
…lized number field component [resolve review comments]

Description:
- add unit test
- resolve review comments
@dsuren1 dsuren1 merged commit ddf127e into geosolutions-it:master May 20, 2024
6 checks passed
@dsuren1 dsuren1 added the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label May 20, 2024
@dsuren1
Copy link
Contributor

dsuren1 commented May 20, 2024

@ElenaGallo Kindly test it in DEV and let us know if it's good to be backported. Thanks!

@ElenaGallo
Copy link
Contributor

Test passed, @mahmoudadel54 please backport to 2024.01.xx. Thanks

mahmoudadel54 added a commit to mahmoudadel54/MapStore2 that referenced this pull request May 21, 2024
@mahmoudadel54
geosolutions-it#10289: Cursor position is shifted incorrectly in Loca…
19774c8 
…lized number field component (geosolutions-it#10312)

* geosolutions-it#10289: Cursor position is shifted incorrectly in Localized number field component
Description:
- fix the jumping cursor to unexpected positions for localized numeric input

* geosolutions-it#10289: Cursor position is shifted incorrectly in Localized number field component
Description:
- replace logic in updateDidMount to onKeyUp
- remove unused comments

* geosolutions-it#10289: Cursor position is shifted incorrectly in Localized number field component
Description:
- remove unused method in IntlNumberFormControl

* geosolutions-it#10136: Search for Map CRS coordinates
Description:
- resolve FE failure in unit test

* geosolutions-it#10289: Cursor position is shifted incorrectly in Localized number field component [resolve review comments]
Description:
- fix issue of on focus effect in NumericInput

* geosolutions-it#10289: Cursor position is shifted incorrectly in Localized number field component [resolve review comments]
Description:
- fix FE issue in onKeyDown in IntlNumberFormControl

* geosolutions-it#10289: Cursor position is shifted incorrectly in Localized number field component [resolve review comments]
Description:
- revert unnecessary change

* geosolutions-it#10289: Cursor position is shifted incorrectly in Localized number field component [resolve review comments]
Description:
- move the fixing logic of cursor position to componentDidUpdate instead of onKeyUp

* geosolutions-it#10289: Cursor position is shifted incorrectly in Localized number field component [resolve review comments]
Description:
- add onKeyUp as a prop into IntlNumberFormControl for any expected future consuming from a parent comp

* geosolutions-it#10289: Cursor position is shifted incorrectly in Localized number field component [resolve review comments]
Description:
- add unit test
- resolve review comments
@mahmoudadel54 mahmoudadel54 mentioned this pull request May 21, 2024
Merged
@mahmoudadel54
Copy link
Collaborator Author

Test passed, @mahmoudadel54 please backport to 2024.01.xx. Thanks

Backport is done ---> #10346

dsuren1 pushed a commit that referenced this pull request May 21, 2024
@mahmoudadel54
#10289: Cursor position is shifted incorrectly in Localized number fi…
05ba09c 
…eld component (#10312) (#10346)

* #10289: Cursor position is shifted incorrectly in Localized number field component
Description:
- fix the jumping cursor to unexpected positions for localized numeric input

* #10289: Cursor position is shifted incorrectly in Localized number field component
Description:
- replace logic in updateDidMount to onKeyUp
- remove unused comments

* #10289: Cursor position is shifted incorrectly in Localized number field component
Description:
- remove unused method in IntlNumberFormControl

* #10136: Search for Map CRS coordinates
Description:
- resolve FE failure in unit test

* #10289: Cursor position is shifted incorrectly in Localized number field component [resolve review comments]
Description:
- fix issue of on focus effect in NumericInput

* #10289: Cursor position is shifted incorrectly in Localized number field component [resolve review comments]
Description:
- fix FE issue in onKeyDown in IntlNumberFormControl

* #10289: Cursor position is shifted incorrectly in Localized number field component [resolve review comments]
Description:
- revert unnecessary change

* #10289: Cursor position is shifted incorrectly in Localized number field component [resolve review comments]
Description:
- move the fixing logic of cursor position to componentDidUpdate instead of onKeyUp

* #10289: Cursor position is shifted incorrectly in Localized number field component [resolve review comments]
Description:
- add onKeyUp as a prop into IntlNumberFormControl for any expected future consuming from a parent comp

* #10289: Cursor position is shifted incorrectly in Localized number field component [resolve review comments]
Description:
- add unit test
- resolve review comments
@ElenaGallo ElenaGallo removed the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsuren1 dsuren1 dsuren1 approved these changes

Assignees

@mahmoudadel54 mahmoudadel54

Labels
bug Internal
Projects
None yet
Milestone
2024.01.01
Development

Successfully merging this pull request may close these issues.

Cursor position is shifted incorrectly in Localized number field component
3 participants
@mahmoudadel54 @dsuren1 @ElenaGallo