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

feat(focus-scope): new component focus-scope component #272

Merged
merged 21 commits into from
Aug 20, 2023

Conversation

Cr0zy07
Copy link
Collaborator

@Cr0zy07 Cr0zy07 commented Aug 5, 2023

Description

Linked Issues

close #268

Additional context

@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2023

Thank you for following the naming conventions! 🙏

@productdevbook
Copy link
Member

You are great ❤️

@Cr0zy07 Cr0zy07 closed this Aug 7, 2023
@Cr0zy07 Cr0zy07 deleted the feat/focusScope/-268 branch August 7, 2023 01:31
@Cr0zy07 Cr0zy07 restored the feat/focusScope/-268 branch August 10, 2023 09:09
@Cr0zy07 Cr0zy07 reopened this Aug 10, 2023
@Cr0zy07 Cr0zy07 marked this pull request as ready for review August 10, 2023 09:23
@productdevbook
Copy link
Member

productdevbook commented Aug 11, 2023

There is a storybook, can you add that too ?

@productdevbook
Copy link
Member

@Cr0zy07 i am same send commit, new struct pnpm dev and pnpm build system. pnpm story same dont change.

// we need to remove the listener after we `dispatchEvent`
container.value?.$el.removeEventListener(AUTOFOCUS_ON_UNMOUNT, onUnmountAutoFocus)

// focusScopesStack.remove(focusScope)
Copy link
Collaborator Author

@Cr0zy07 Cr0zy07 Aug 15, 2023

Choose a reason for hiding this comment

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

This line is not working properly; it causes an infinite loop, and triggers the warning '[Vue warn]: Maximum recursive updates exceeded.'

As a consequence, unexpected behavior arises within the 'Multiple' and 'WithOptions' stories.

In the 'Multiple' scenario, when both traps are open, clicking on the outer input moves the focus event to it. Meanwhile, in the 'WithOptions' scenario, the 'age' field sometimes got focus, and other times it does not.

Could you please check it @productdevbook

Copy link
Member

Choose a reason for hiding this comment

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

infitinite loop test

This is the answer to the question here, I committed it.

  afterEach(() => wrapper.unmount());

age field focus bugs i see. I'm looking after

Copy link
Collaborator Author

@Cr0zy07 Cr0zy07 Aug 17, 2023

Choose a reason for hiding this comment

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

The infinite loop that I mentioned was actually in the second watchEffect not in the test. However, I tried two approaches yesterday and it fixed the first issue related to the 'Multiple' story.

watchEffect(async (onInvalidate) => {
  ...
  if (!hasFocusedCandidate) {
    await nextTick()
    ...
  }
 })

or

watchSyncEffect((onInvalidate) => { ... })

Both solutions work, but as I know the second one is unsafe to use.

For the age field focus bug, I was thinking it is because the component has not mounted yet, but even if I changed the v-if to v-show it does not work! :(

Copy link
Member

Choose a reason for hiding this comment

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

You were getting this error only in tests, right ? because I never came across it in the browser.

Copy link
Collaborator Author

@Cr0zy07 Cr0zy07 Aug 17, 2023

Choose a reason for hiding this comment

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

No, that was in the vue playground. Because when I was running the 'Multiple' story the browser crashed. It was not always crashing, there are steps if you follow the warning message will appear or the browser will crash.

  • Open the Trap 1.
  • Open the Trap 2.
  • Close the Trap 1.
  • Reopen the Trap 1.

Copy link
Collaborator Author

@Cr0zy07 Cr0zy07 Aug 19, 2023

Choose a reason for hiding this comment

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

Do you mean that by removing the v-if the age field is getting focus?, becouse I tried before with v-show and it doesn't!

v-if is removing the age field from the dom. So, if it works with you, we need to add onMounted to the age ref.

UPDATE

I just remembered that it only works if the form is actually showing, but since v-show does not remove the age field from the dom it should work!

Also, I noticed that the Ridax story does not move the focus from the age field to the first field after toggling the check box, but our does.

Copy link
Member

Choose a reason for hiding this comment

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

i am send new commit if

  if (focusOnMount.value !== true) {
    event.preventDefault()
    if (ageFieldRef.value)
      ageFieldRef.value?.focus()
  }

change working but this is not similar to radix.

Copy link
Collaborator Author

@Cr0zy07 Cr0zy07 Aug 19, 2023

Choose a reason for hiding this comment

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

I will check it when I reach home. I still need to move the exported type from the beginning of focus-scope file to the utilis file.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

if you think of something different you can post a new pr

@productdevbook productdevbook marked this pull request as draft August 17, 2023 05:42
@productdevbook productdevbook marked this pull request as ready for review August 20, 2023 04:51
Copy link
Member

@productdevbook productdevbook left a comment

Choose a reason for hiding this comment

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

thank you ❤️

@productdevbook productdevbook changed the title feat: focus-scope component feat(focus-scope): new component focus-scope component Aug 20, 2023
@productdevbook
Copy link
Member

Some of your tests are not working because we passed happy-dom from jsdom. If you can get them working can you post them in the next pr?

@productdevbook productdevbook merged commit b8d700d into oku-ui:main Aug 20, 2023
6 checks passed
@github-actions github-actions bot mentioned this pull request Aug 20, 2023
@github-actions github-actions bot mentioned this pull request Sep 11, 2023
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.

focus-scope package
2 participants