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

[chore] Refactor all classes to hooks #822

Merged
merged 21 commits into from
Jan 8, 2022
Merged

[chore] Refactor all classes to hooks #822

merged 21 commits into from
Jan 8, 2022

Conversation

dej611
Copy link
Member

@dej611 dej611 commented Dec 26, 2021

PR Checklist

  • My branch is up-to-date with the Upstream master branch.
  • The unit tests pass locally with my changes (if applicable).
  • I have added tests that prove my fix is effective or that my feature works (if applicable).
  • I have added necessary documentation (if appropriate).

Short description of what this resolves:

Complete the code refactoring from class to hooks.

  • add unit tests for classes rewritten with hooks

Some extras:

  • Created a parallel hook useFontLoader to use as replacement of the FontLoader component.
    • Update documentation
  • Rewrote some utility functions from reactstrap as internal functions
    • Add some unit tests
  • Rewrote some constant imports from reactstrap as static internal values
  • Attempt to remove the SSR check in favour of conditional check (?!)
  • Removed unused and undocumented PasswordInput 🔥
    • Add doc for migration just in case somebody found it and was using it

@vercel
Copy link

vercel bot commented Dec 26, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/dip-trasformazione-digitale/design-react-kit/C9HHJtWGSBBvriZhzr9eJhFLWzNN
✅ Preview: https://design-react-kit-git-feature-hooks-dip-trasformazione-digitale.vercel.app

@dej611 dej611 changed the title [chore]] Refactor all classes to hooks [chore] Refactor all classes to hooks Dec 26, 2021
@dej611
Copy link
Member Author

dej611 commented Dec 27, 2021

Just for SEO context I'll address here how I made it pass the treeshake check with agadoo:

  • Moved the remaining React components implemented with class with their hook version (see also Not tree shakable React Pure Component Rich-Harris/agadoo#12 )
  • Re-implemented some initial code used for SSR environment check in alternative way (like use a conditional check for its client-side method)
  • Re-written async/await code into thenables (this removed the Babel polyfill about iterators)

export type AccordionBodyProps = Partial<TransitionProps> & {
tag?: ElementType;
className?: string;
active?: boolean;
onToggle?: () => void;
};

// Taken from Reactstrap utils.js which are tken from bootstrap sass
export const TransitionTimeouts = {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we move these to some shared const files and then consume these? These appear to be generic

};

// taken from Reactstrap utils.js
export const TransitionsKeys = [
Copy link
Member

Choose a reason for hiding this comment

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

Same as above^

@codecov
Copy link

codecov bot commented Dec 30, 2021

Codecov Report

Merging #822 (32b876b) into master (1274100) will increase coverage by 1.77%.
The diff coverage is 83.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #822      +/-   ##
==========================================
+ Coverage   95.28%   97.05%   +1.77%     
==========================================
  Files         237      237              
  Lines        2608     2617       +9     
  Branches      476      471       -5     
==========================================
+ Hits         2485     2540      +55     
+ Misses        123       77      -46     
Flag Coverage Δ
unittests 97.05% <83.79%> (+1.77%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Forward/Forward.tsx 83.33% <0.00%> (+18.62%) ⬆️
src/Rating/Rating.tsx 100.00% <ø> (ø)
src/Input/utils.tsx 86.74% <58.82%> (-5.91%) ⬇️
src/Accordion/AccordionBody.tsx 63.26% <62.79%> (-9.08%) ⬇️
src/Input/Input.tsx 79.26% <77.63%> (+3.98%) ⬆️
src/Input/TextArea.tsx 89.18% <88.23%> (+76.42%) ⬆️
src/Header/HeaderBrand.tsx 96.66% <96.15%> (-0.31%) ⬇️
src/FontLoader/FontLoader.tsx 100.00% <100.00%> (ø)
src/Header/HeaderContent.tsx 100.00% <100.00%> (ø)
src/Header/HeaderContext.tsx 100.00% <100.00%> (ø)
... and 15 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 1274100...32b876b. Read the comment docs.

@dej611
Copy link
Member Author

dej611 commented Jan 1, 2022

@baldoalessandro this PR is passing the treeshake check but the bash script is not catching that up correctly. Can you have a look at that?

@baldoalessandro
Copy link
Contributor

baldoalessandro commented Jan 4, 2022

@dej611 Sure thing, if I find something should I add commits here or open a new PR?

Update

I've found the issue and made a PR to test it in my fork, you can see it here baldoalessandro#5
Let me know if I should open a PR against master or this branch, sorry for the inconvenience 😊

@dej611
Copy link
Member Author

dej611 commented Jan 5, 2022

@dej611 Sure thing, if I find something should I add commits here or open a new PR?

Update

I've found the issue and made a PR to test it in my fork, you can see it here baldoalessandro#5 Let me know if I should open a PR against master or this branch, sorry for the inconvenience 😊

You can create a PR here that targets this PR/branch.

@dej611
Copy link
Member Author

dej611 commented Jan 5, 2022

@baldoalessandro I've borrowed your fix for #830 as I was already fixing some stuff in the area.
If you prefer to submit your own PR I'll revert the changes and let you push your own PR.

EDIT:
I've tested your fix and it is still buggy. It returns that the PR is tree-shakeable while it's not.

@baldoalessandro
Copy link
Contributor

@dej611 no problem 👍

EDIT:
I've tested your fix and it is still buggy. It returns that the PR is tree-shakeable while it's not.
Do you mean that the check in the PR passed in both cases or there was a problem with the exit-code parsing?
In the first case I didn't know you wanted to fail the check but I've seen you already added:

      - name: Fail if cannot tree-shake
        if: ${{ steps.agadoo.outputs.status == 'not tree-shakeable' }}
        run: exit 1

in case there are still errors I'm happy to help you!

@dej611
Copy link
Member Author

dej611 commented Jan 5, 2022

Do you mean that the check in the PR passed in both cases or there was a problem with the exit-code parsing?
In the first case I didn't know you wanted to fail the check but I've seen you already added:

It was reporting success all the times, as RET was not correctly capturing the correct output from agadoo. So I rewrote the script a bit to read the last line of the output

@dej611 dej611 marked this pull request as ready for review January 7, 2022 17:13
@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2022

Check tree-shakeability for 3751833: 🌳 tree-shakeable

Agadoo stdout
import 'reactstrap';
import 'react-use-navscroll';
import 'react';
import 'classnames';
import 'react-transition-group';
import 'react-stickup';
import 'is-number';
import 'bootstrap-italia/dist/assets/resizer-3x2.svg';
import 'react-select';
import 'react-toastify';
Agadoo stderr
npx: installed 6 in 2.092s
'reactstrap' is imported by dist/esm/index.js, but could not be resolved  treating it as an external dependency
'react-use-navscroll' is imported by dist/esm/index.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/index.js, but could not be resolved  treating it as an external dependency
'classnames' is imported by dist/esm/index.js, but could not be resolved  treating it as an external dependency
'react-transition-group' is imported by dist/esm/index.js, but could not be resolved  treating it as an external dependency
'react-stickup' is imported by dist/esm/index.js, but could not be resolved  treating it as an external dependency
'is-number' is imported by dist/esm/index.js, but could not be resolved  treating it as an external dependency
'bootstrap-italia/dist/assets/resizer-3x2.svg' is imported by dist/esm/index.js, but could not be resolved  treating it as an external dependency
'react-select' is imported by dist/esm/index.js, but could not be resolved  treating it as an external dependency
'react-toastify' is imported by dist/esm/index.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItArrowDownCircle-ed01958f.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItArrowDown-f935c404.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItArrowDownTriangle-43e14d4e.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItArrowLeftCircle-022f3cd0.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItArrowLeftTriangle-85ad190a.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItArrowLeft-0eb29da6.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItArrowRightCircle-004ceffe.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItArrowRightTriangle-2270d70b.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItArrowRight-ea38dca9.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItArrowUpCircle-c58494d6.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItArrowUpTriangle-a3d220d7.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItArrowUp-4da728d9.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItBan-eed8efa1.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItBehance-7cfa1aa8.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItBookmark-bbd0a6eb.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItBox-c57a00d9.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItBurger-a48f8693.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItCalendar-4ced1f4f.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItCamera-ea37a275.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItCard-08e35364.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItChartLine-4d19c837.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItCheckCircle-80464fda.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItCheck-b13dfb47.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItChevronLeft-9e6f642c.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItChevronRight-5d8994f9.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItClip-5625b723.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItClock-e99f09d6.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItCloseBig-48385084.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItCloseCircle-e9aa8a4e.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItClose-da35711d.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItCodeCircle-f1a1174b.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItCollapse-7eeb99ff.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItComment-a939362d.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItCopy-a064bc7e.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItDelete-49e04a94.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItDesignersItalia-a7e1e47e.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItDownload-223fddb3.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItError-6411ebd1.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItExchangeCircle-b2e80da1.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItExpand-b9bf051e.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItExternalLink-3839a562.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItFacebookSquare-5d1a9e56.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItFacebook-a587f690.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItFile-713e7067.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItFiles-f13caaef.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItFlag-818a6c3e.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItFlickrSquare-f0eccb5c.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItFlickr-a1ae3080.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItFolder-46e3c66c.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItFullscreen-fd759316.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItFunnel-fcb6dbeb.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItGithub-ea460a17.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItHearing-6a6a24b1.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItHelpCircle-5ac93785.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItHelp-271b034b.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItHorn-779c3634.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItInbox-7d3124b3.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItInfoCircle-ecce1964.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItInstagram-a22a92f7.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItKey-27cc1af3.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItLessCircle-5fd48c2f.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItLink-d67a489f.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItLinkedinSquare-0c3cdc22.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItLinkedin-9cfc3fae.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItList-8d490e85.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItLock-9d50d11c.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItLocked-f6436368.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItMail-df1e741a.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItMapMarkerCircle-563223f9.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItMapMarkerMinus-edb3574a.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItMapMarkerPlus-da25d79c.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItMapMarker-b63f733c.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItMaximizeAlt-3b44dd74.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItMaximize-8a293b02.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItMediumSquare-6de2cbd2.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItMedium-fda5c30a.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItMinimize-eb2ef1b9.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItMinusCircle-a4ad8e7e.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItMinus-f14df53a.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItMoreActions-1a78b8bd.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItMoreItems-994f3a0f.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItNote-e3b62893.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItOpenSource-63a1d44f.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItPa-8303599f.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItPasswordInvisible-6905b324.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItPasswordVisible-24431639.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItPencil-653ba0f7.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItPiattaforme-88f0e33e.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItPin-6afe491b.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItPlug-307dee08.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItPlusCircle-088c90ce.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItPlus-adeccea9.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItPresentation-1145d3ea.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItPrint-385582c1.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItRefresh-d4630557.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItRestore-d4706243.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItRssSquare-cf0e5940.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItRss-546f5827.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItSearch-78977d22.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItSettings-26f72896.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItShare-3f639a55.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItSoftware-59a24bb3.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItStarFull-e7491d5b.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItStarOutline-802e6204.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItTool-e87380ab.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItTwitterSquare-3f1a6138.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItTwitter-47ea4eb7.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItUnlocked-ce1c89be.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItUpload-b224e070.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItUser-dc34de8e.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItVideo-62f9a455.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItWarningCircle-cb2a4dd8.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItWarning-c5264ce7.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItWhatsappSquare-f557e07b.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItWhatsapp-02928732.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItWifi-ce1648c9.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItYoutube-291b7a7a.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItZoomIn-35bcda8e.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItZoomOut-dcb3a379.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItTelephone-3447b069.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItTelegram-7bb55bce.js, but could not be resolved  treating it as an external dependency
'react' is imported by dist/esm/ItTeamDigitale-5a025df7.js, but could not be resolved  treating it as an external dependency
Success! dist/esm/index.js is fully tree-shakeable

@dej611 dej611 merged commit 6332de5 into master Jan 8, 2022
@astagi astagi deleted the feature/hooks branch March 15, 2024 09:14
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