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

Fix issue for bug #452. Be able to delete / hit backspace on highligh… #473

Merged
merged 5 commits into from
Jun 2, 2021

Conversation

rossjackson
Copy link
Contributor

@rossjackson rossjackson commented Dec 14, 2020

…t when format attribute is enabled

Describe the issue/change

There is an issue stated on #452 when a user highlights a group of values that has a format character that it won't delete those grouped values.

Describe the changes proposed/implemented in this PR

The change is to let user be able to delete these grouped values but retaining the format character.

Link Github issue if this PR solved an existing issue

#452

Please check which browsers were used for testing

  • [X ] Chrome
  • [] Safari (OSX)
  • Safari (iOS)
  • Firefox

@changelogg
Copy link

changelogg bot commented Dec 14, 2020

Hey! Changelogs info seems to be missing or might be in incorrect format.
Please use the below template in PR description to ensure Changelogg can detect your changes:
- (tag) changelog_text
or
- tag: changelog_text
OR
You can add tag in PR header or while doing a commit too
(tag) PR header
or
tag: PR header
Valid tags: added / feat, changed, deprecated, fixed / fix, removed, security, build, ci, chore, docs, perf, refactor, revert, style, test
Thanks!
For more info, check out changelogg docs

Comment on lines 642 to 666
const deletedValues = lastValue.substr(start, end - start);
const hasNonNumeric = !![...deletedValues].find((deletedVal, idx) => this.isCharacterAFormat(idx + start, lastValue));

// if it has, only remove the numeric portion
if(hasNonNumeric) {
const deletedValuePortion = lastValue.substr(start)
const recordIndexOfFormatCharacters = {};
const resolvedPortion = [];
[...deletedValuePortion].forEach((currentPortion, idx) => {
if(this.isCharacterAFormat(idx + start, lastValue)){
recordIndexOfFormatCharacters[idx] = currentPortion;
} else if (idx > deletedValues.length - 1) {
resolvedPortion.push(currentPortion);
}
})

Object.keys(recordIndexOfFormatCharacters).forEach(idx => {
if(resolvedPortion.length > idx){
resolvedPortion.splice(idx, 0, recordIndexOfFormatCharacters[idx]);
} else {
resolvedPortion.push(recordIndexOfFormatCharacters[idx])
}
})

value = lastValue.substr(0, start) + resolvedPortion.join('');
Copy link
Owner

Choose a reason for hiding this comment

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

The problem is the format character can have numeric value in itself.
This logic makes the assumption that there will be no numeric char on format itself.

Copy link
Contributor Author

@rossjackson rossjackson Dec 18, 2020

Choose a reason for hiding this comment

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

My comment was misleading. I wasn't checking if the characters that were removed were numeric. I corrected it and also created unit test to back it up

… tests to back it up. Numeric format characters will still be maintained
Comment on lines 642 to 645
const hasNonNumeric = !![...deletedValues].find((deletedVal, idx) => this.isCharacterAFormat(idx + start, lastValue));

// if it has, only remove characters that are not part of the format
if(hasNonNumeric) {
Copy link
Owner

Choose a reason for hiding this comment

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

hasNonNumeric variable name is confusing as format char can have number as well. Can we rename it to formatGotDeleted.

test/library/input.spec.js Show resolved Hide resolved
test/library/input.spec.js Outdated Show resolved Hide resolved
@s-yadav
Copy link
Owner

s-yadav commented Jun 2, 2021

Something wrong with your prettier. Can you pull code from main repo. There are some conflicts.

@rossjackson
Copy link
Contributor Author

My master was outdated, I had to fetch latest and fix merge issues

Co-authored-by: Sudhanshu Yadav <[email protected]>
@s-yadav s-yadav merged commit 151b162 into s-yadav:master Jun 2, 2021
s-yadav added a commit that referenced this pull request Jun 7, 2021
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