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

[EuiDatePicker] react-datepicker to src/; use EUI services #5339

Merged
merged 26 commits into from
Nov 17, 2021

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Nov 1, 2021

Summary

Beginning of #3901

  • 📦 Moves react-datepicker inside src/
    • Renames .jsx files to .js (up for discussion)
    • Allows for direct import of DatePicker (no separate build step)
  • ✨ Allows EuiDatePicker to use EUI components
    • EuiPopover
    • EuiFocusTrap
    • EuiOutsideClickDetector
    • EuiScreenReaderOnly
    • Negates all react-datepicker dependencies

Discuss

  • 🗑️ More removals from date_picker/react-datepicker/
    • All build- and repo-related files are not necessary
    • .babelrc and package.json files interfere with EUI build and need to be renamed
    • Tests and docs?
  • 📄 License
    • Am I missing anything from a license perspective?
    • Is keeping LICENSE in the directory enough?
  • ⚠️ Breaking changes
    • I think I avoided all breaking changes with the exception of popperProps, which was spread onto the Popper component but not documented in EUI

Fixes #3476, closes #3850

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles

- [ ] Added documentation

  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5339/

@thompsongl thompsongl marked this pull request as ready for review November 1, 2021 17:35
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5339/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5339/

@cee-chen
Copy link
Member

cee-chen commented Nov 3, 2021

Sorry just saw your PR description request for discussion:

🗑️ More removals from date_picker/react-datepicker/
All build- and repo-related files are not necessary
.babelrc and package.json files interfere with EUI build and need to be renamed
Tests and docs?

++ to removing stuff that isn't actually sources:

  • I'd nuke all the root-level build/repo related files except for README and LICENSE (just IMO) as well as scripts/
  • I'd definitely nuke docs-site/ and examples/hello-world/
  • I'm torn on docs/ and test/ - those seem mildly useful. And I just used docs/ (for example) to compare against the OG react-datepicker to confirm that it had onCalendarClose vs. EUI which did not. Since those 2 folders aren't overly hefty I think I'd vote for keeping them in and copying them manually if and when we go to react-datepicker for updates (I don't actually know if we ever even do that or if that's reasonable).

📄 License
Am I missing anything from a license perspective?
Is keeping LICENSE in the directory enough?

IIRC should be fine, but @chandlerprall certainly knows more than me!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5339/

@cchaos
Copy link
Contributor

cchaos commented Nov 10, 2021

I'd definitely nuke docs-site/ and examples/hello-world/

Yes please!!!

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5339/

@thompsongl
Copy link
Contributor Author

thompsongl commented Nov 10, 2021

Update: Deleted everything except for src/ (obviously), test/, docs/, LICENSE, and README. Also renamed package.json to original.package.json (I like the idea of keeping it, but it needs to be renamed because babel is weird)

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5339/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5339/

@thompsongl
Copy link
Contributor Author

Hmm, something broke with the month / year dropdowns where the options are no longer selectable.

Fixed!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5339/

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

I clicked through our docs and couldn't find any other issues. I can't wait to convert these to real EUI components and no more positioning hacks!

src/components/date_picker/date_picker.tsx Outdated Show resolved Hide resolved
src/components/date_picker/react-datepicker/src/index.js Outdated Show resolved Hide resolved
thompsongl and others added 2 commits November 11, 2021 15:46
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5339/

Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Sweeet! I didn't find anything else personally on a second pass/skim/+quick QA - LGTM!

Also hot DANG

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5339/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Can we exclude the docs & test directories from being published, similar to how the postinstall script is managed in .npmignore ?

Our license guidelines aren't very clear here, but I think it's safest and clearest to enforce having react-datepicker's MIT license head each file under its docs, src, and test dirs

@thompsongl
Copy link
Contributor Author

thompsongl commented Nov 16, 2021

I think it's safest and clearest to enforce having react-datepicker's MIT license head each file under its docs, src, and test dirs

c96c95e inlines the license in nested react-datepicker directories

Can we exclude the docs & test directories from being published, similar to how the postinstall script is managed in .npmignore ?

No further changes necessary to accomplish this. .npmignore already omits everything but .scss files, and the compile script ignores all other potentially matching files. Also confirmed the dist/ bundle does not include test/ or docs/ files.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5339/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5339/

@thompsongl
Copy link
Contributor Author

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5339/

@chandlerprall
Copy link
Contributor

c96c95e inlines the license in nested react-datepicker directories

Confirmed!

No further changes necessary to accomplish this. .npmignore already omits everything but .scss files, and the compile script ignores all other potentially matching files. Also confirmed the dist/ bundle does not include test/ or docs/ files.

Ah, perfect! Tested with a yarn build locally to triple check and everything looks good to me ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants