search

motss / app-datepicker

Datepicker element built with Google's lit and Material Design 2021
https://npm.im/app-datepicker
MIT License
180 stars 51 forks source link

💥 🚚 Upgrade to Polymer 3 #125

Closed jordanfinners closed 5 years ago

jordanfinners commented 6 years ago

Question @motss <script src="/node_modules/web-animations-js/web-animations-next-lite.min.js"></script> <script src="/node_modules/intl/dist/Intl.complete.js"></script> Need these to polyfill the browser so the animations and intl work, but can't es6 import them. Do you think we should add them to the document head when initialising the element or just make users add them themselves when using the component?

This change is Reviewable

motss commented 6 years ago

Web Animations and Intl are optional polyfills and only be loaded when necessary. It is recommended to load them at the top level document. README.md needs to document those.

motss commented 6 years ago

A few thoughts that worth considering as part of this PR:

  1. TypeScript everything
  2. Code formatter with clang like what lit-html does
  3. To drop @polymer/neon-animation in favor of native Web Animations APIs
jordanfinners commented 6 years ago

Will update the readme to reflect the optional importing of these dependencies - https://github.com/motss/app-datepicker/pull/125#issuecomment-443475080

For this comment - https://github.com/motss/app-datepicker/pull/125#issuecomment-443475212 All 3 I think are sensible, however I think that would be expanding this (already large) PR. Perhaps it could be done in steps rather than doing it all in one go in this piece of work?

Will work through your other PR comments and Codacy/codebeat reports 👍

jordanfinners commented 6 years ago

Also will you be putting this onto npm once this PR is merged? As can't bower install it anymore once this is merged and the README will need updating.

And how did you test all the browser compatibility?

motss commented 5 years ago

Cool. Let's move those comments into another project. We're going to use npm starting Polymer 3. I'll publish the element once this is merged. BTW, the warnings and errors can be ignored from the codebeat.

motss commented 5 years ago

We rely on Travis for the cross browsers teatings. I decided to add Saucelabs back again which was removed earlier.

jordanfinners commented 5 years ago

@motss updated this PR with your feedback and some of the code check feedback. Are there any other PR's you want to get merged in before this one?

motss commented 5 years ago

I'll take a look on the latest changes first then decide to work on merging some of the fixes later in another PR since it's hard to do that for this PR. Plus I want to make a NPM publish for this element.

jordanfinners commented 5 years ago

Sounds good! :D The Gitmoji Police checker lies 😂

motss commented 5 years ago

@jordanfinners Thanks for your contribution. BTW, I noticed that you forced-push a commit for some reasons. Please don't do that as we need to find out what you have changed in each commit. In such way, we don't have to read every single changes and not knowing what you've made subsequently.

LGTM, I'll proceed to merging this PR!

jordanfinners commented 5 years ago

Thanks @motss !

Yeah I rebase my commits down to just one commit and force pushed it, just to keep it tidy :)

jordanfinners commented 5 years ago

Let me know when you've got it on NPM! :D

motss commented 5 years ago

@jordanfinners Actually you don't have to do the rebase on your side. Github author can do that while performing the merge.

motss commented 5 years ago

@jordanfinners Not so soon to NPM publishing. I'm working on the tests and it is broken on some browsers. Feel free to help fixing them.

jordanfinners commented 5 years ago

Okay @motss, let me know what's broke and we can split it out between us?

motss commented 5 years ago

Test case test for update to reflect external date change » should show correct output date when 'showLongDate' is falsy is broken on Safari 9 - 12

jordanfinners commented 5 years ago

Ah I don't have Safari to help you on that one. Is there anything else you'd like testing/help on?

motss commented 5 years ago

Same here. I'm able to test on Macbook a couple days later in this week. I don't mind NPM publishing it first as alpha version in the meantime.

On Tue, Dec 4, 2018, 2:18 AM Jordan Finneran notifications@github.com wrote:

Ah I don't have Safari to help you on that one. Is there anything else you'd like testing/help on?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/motss/app-datepicker/pull/125#issuecomment-443812492, or mute the thread https://github.com/notifications/unsubscribe-auth/AKHcj_N7dZb_CbFOv-5ofWgNeE8HdrBYks5u1Wr1gaJpZM4Ywyt_ .

jordanfinners commented 5 years ago

Yeah an alpha version on npm would be good! Let me know if you need any help with anything else! :)

motss commented 5 years ago

Published! https://www.npmjs.com/package/app-datepicker

jordanfinners commented 5 years ago

Does this issue need closing? (I checked but don't think I can 🙈) https://github.com/motss/app-datepicker/issues/112

Thank you for getting this through so quickly!

motss commented 5 years ago

I already closed the issue. Thanks for the effort for making this happen. Do let me know if the datepicker is working.