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

fix: Mentions doesnt work on chrome and on firefox #280

Merged
merged 8 commits into from
Sep 19, 2024
Merged

fix: Mentions doesnt work on chrome and on firefox #280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ac6176e
fix:fixing mention dosnt work on chrom and on firfox when using azer…
ilyassBZ Sep 14, 2024
99f27f8
feat:addinf test case for azerty keys
ilyassBZ Sep 14, 2024
d1255a7
fix:version
ilyassBZ Sep 14, 2024
2b013dc
Merge branch 'master' into master
ilyassBZ Sep 14, 2024
d09fe09
Update src/Mentions.tsx
ilyassBZ Sep 19, 2024
713706b
Update tests/FullProcess.spec.tsx
ilyassBZ Sep 19, 2024
32714f2
Update tests/FullProcess.spec.tsx
ilyassBZ Sep 19, 2024
507362d
Update src/Mentions.tsx
ilyassBZ Sep 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/Mentions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,10 +391,13 @@ const InternalMentions = forwardRef<MentionsRef, MentionsProps>(
const matchOption = !!getOptions(nextMeasureText).length;

if (validateMeasure) {
// adding AltGraph also fort azert keyboard
if (
key === nextMeasurePrefix ||
key === 'Shift' ||
which === KeyCode.ALT ||
key === 'AltGraph' ||
Copy link
Member

@afc163 afc163 Sep 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@Jarryxin Is this correct? Should #281 already resolve the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2024-09-14_11-36
....

Copy link
Contributor

Choose a reason for hiding this comment

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

@Jarryxin Is this correct? Should #281 already resolve the problem?

可能确实不一样，我是用港版mac改成法语，测试alt是 18，在我修改的版本发邮件给提问者，他说是正确的。是否应该两种alt都兼容？

Maybe it's different. I changed from using the Hong Kong version of Mac to French, and the alt test was 18. In the revised version I provided to the questioner, he said it was correct. Should both alt versions be compatible?

Copy link
Contributor

Choose a reason for hiding this comment

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

ant-design/ant-design#50795 (comment) in this comment , the alt keycode is 18

Copy link
Contributor Author

@ilyassBZ ilyassBZ Sep 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Im azerty user we have 2 Alt keys the one u talked about and the other one it AltGr that it been user to do @ cus in azerty keyboard just normal Alt +  à dosnt do anything AltGr does
I guess in mac in diffrent or something
In azert keyboard we have left alt whiche it all keyboard and the right AltGr with it only in azerty keyboard

Copy link
Contributor Author

@ilyassBZ ilyassBZ Sep 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

U can merge both do if u wanna satay safe in case ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i guess we should merge both of them do

Copy link
Contributor Author

@ilyassBZ ilyassBZ Sep 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

cus in ur coode u looking for the key number which it 18 OR 225 depending on keyboard but key name gonna stay the same which it AltGraph it dosnt change ... that's make using key=== "AltGraph" moore affection than using which key number but im fine with merging both of them if thats gonna suit u @yoyo837 @Jarryxin

Copy link
Member

Choose a reason for hiding this comment

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

KeyCode.ALT or AltGraph? Which one is correct?

Copy link
Member

@afc163 afc163 Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i guess we should merge both of them do

Should we keep KeyCode.ALT?

ilyassBZ marked this conversation as resolved.
Show resolved Hide resolved

mergedMeasuring ||
(nextMeasureText !== mergedMeasureText && matchOption)
) {
Expand Down
20 changes: 20 additions & 0 deletions tests/FullProcess.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,26 @@ describe('Full Process', () => {
expect(onChange).toBeCalledWith('1 @bamboo 2');
});

it('azerty Keyboards ', () => {
const onChange = jest.fn();
const { container } = createMentions({ onChange });
simulateInput(container, '@');

// keyCode for ALTGR
fireEvent.keyUp(container.querySelector('textarea'), {
keyCode: 'AltGraph',
which: 225,
});
expectMeasuring(container);

fireEvent.keyDown(container.querySelector('textarea'), {
keyCode: KeyCode.ENTER,
which: KeyCode.ENTER,
});

expect(onChange).toBeCalledWith('@bamboo ');
});
ilyassBZ marked this conversation as resolved.
Show resolved Hide resolved

it('reuse typed text', () => {
const onChange = jest.fn();
const { container } = createMentions({ onChange });
Expand Down