Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

chore: update Ref dependency to latest version #4023

Conversation

jluxenberg
Copy link
Contributor

The @stardust-ui/{react-component-event-listener, react-component-ref} components have been moved to @fluentui/{react-component-event-listener, react-component-ref} (https://github.com/stardust-ui/react now redirects to https://github.com/microsoft/fluent-ui-react)

Change dependency so that the new packages are used, and upgrade to their latest versions.

One wrinkle: toRefObject() was removed from react-component-ref (see microsoft/fluent-ui-react#2287). I've rewritten the code that depended on this function.

The @stardust-ui/{react-component-event-listener, react-component-ref} components have been moved to @fluentui/{react-component-event-listener, react-component-ref}.

Change dependency so that the new packages are used, and upgrade to their latest versions.

One wrinkle: `toRefObject()` was removed from `react-component-ref` (see microsoft/fluent-ui-react#2287). I've rewritten the code that depended on this function.
@welcome
Copy link

welcome bot commented Aug 5, 2020

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

  • Run yarn lint locally to catch formatting errors. This will fix some errors automatically, commit and push any changes.
  • Run yarn test locally to catch errors. This ensures all components still behave as they should.
  • Run yarn start to run the doc site locally and try a few pages, ensuring everything is in good working order.
  • Include tests when adding/changing behavior.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2020

Codecov Report

Merging #4023 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4023   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files         183      183           
  Lines        3276     3276           
=======================================
  Hits         3271     3271           
  Misses          5        5           
Impacted Files Coverage Δ
src/addons/Portal/Portal.js 100.00% <ø> (ø)
src/addons/Portal/PortalInner.js 100.00% <ø> (ø)
src/addons/TextArea/TextArea.js 100.00% <ø> (ø)
src/behaviors/Visibility/Visibility.js 100.00% <ø> (ø)
src/elements/Button/Button.js 95.55% <ø> (ø)
src/elements/Input/Input.js 100.00% <ø> (ø)
src/lib/hooks/useClassNamesOnNode.js 100.00% <ø> (ø)
src/modules/Checkbox/Checkbox.js 100.00% <ø> (ø)
src/modules/Dimmer/DimmerInner.js 100.00% <ø> (ø)
src/modules/Dropdown/Dropdown.js 100.00% <ø> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dba2f1...734dfa8. Read the comment docs.

@layershifter
Copy link
Member

@jluxenberg that's great 🎉

However, can we also partially revert #3774 to include Ref*, examples and its functions in our codebase? Per #3915 for consumers it is unclear that Ref component exists at all and what should be used.

const nodeRef = getNodeRefFromProps(this.props)

nodeRegistry.del(nodeRef, this)
nodeRegistry.emit(nodeRef, handleClassNamesChange)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that it's valid to remove this part as it solves duplicated classnames issue, for example:

<MountNode className="foo" node={document.body} />
<MountNode className="foo" node={document.body} />
<MountNode className="bar" node={document.body} />

Should add only foo bar.

This also handles unmount scenario: when one of MountNode components with className="foo" will be removed - foo should still be defined on document.body as there is another component that defines it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it; thanks for this additional test case. I will take a look and see if my change can be reworked.

Copy link
Member

Choose a reason for hiding this comment

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

@jluxenberg do not spend cycles on MountNode, I am working on a hook that will replace it...

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.

I merged the original PR and solved merge conflicts in this. Can you please check? It also seems that we have a single comment left 👍

…React into jluxenberg/stardust-ui-is-now-fluentui

� Conflicts:
�	src/addons/MountNode/MountNode.js
�	src/addons/MountNode/lib/getNodeRefFromProps.js
�	test/specs/lib/hooks/NodeRegistry-test.js
package.json Outdated Show resolved Hide resolved
@layershifter
Copy link
Member

However, can we also partially revert #3774 to include Ref*, examples and its functions in our codebase? Per #3915 for consumers it is unclear that Ref component exists at all and what should be used.

We can address this one is a separate PR if you're interested 🤔

@layershifter layershifter changed the title chore(MountNode|Sidebar): update Ref dependency to latest version chore: update Ref dependency to latest version Aug 17, 2020
@layershifter layershifter merged commit bf87dd2 into Semantic-Org:master Aug 17, 2020
@welcome
Copy link

welcome bot commented Aug 17, 2020

Congrats on merging your first pull request! 🎉🎉🎉

robot victory dance

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants