search

JosephusPaye / Keen-UI

A lightweight Vue.js UI library with a simple API, inspired by Google's Material Design.
https://josephuspaye.github.io/Keen-UI/
MIT License
4.1k stars 438 forks source link

Migrate to Vue 3! #538

Closed kilobyte2007 closed 2 years ago

kilobyte2007 commented 2 years ago

I have taken the time and migrated the project to Vue 3. The functionality is mostly intact as only dependency upgrades and the necessary migration refactoring were performed. Here's what was done:

  1. Migrated all of the Vue code based on https://v3-migration.vuejs.org/
  2. Switched the dev server to use vite
  3. Upgraded webpack from 2 to 5
  4. Upgraded several other dependencies
  5. Restructured the build configurations to work with webpack 5
  6. Adjusted the documentation to reflect changes

Please let me know if I missed something. I'd be happy if this PR was merged so I could move our own project to Vue 3.

Besides the above, I have several other bugfixes I'd like to discuss, but that's probably for a different thread.

kilobyte2007 commented 2 years ago

Hey @JosephusPaye, Thanks a lot for the detailed commentaries, I have fixed almost all of the issues as well as the bugs and warnings you mentioned.

  1. As for using vite for all the builds, I agree with that, we'll have to look into this, as this might make more sense, I am just not that familiar yet with vite and rollup to configure it as confidently as I can configure webpack.
  2. The devtool still works even without the flag when running the production build of docs.
  3. As for the breaking changes - yes, would it be better to document them in the release notes/changelog somewhere?
  4. Also I added a fix for the calendar as it was inheriting some global styles for table from the docs but it was looking differently when using KeenUI in our project as that style was not included in the bundle. Maybe it makes sense to never use global table and td styles in the docs CSS so that we can be sure that the docs reflect everything correctly.
  5. The UiSelect issue I managed to fix as it seems that both the ui-popover and the ui-select were listening to the outside click event, it seems to work perfectly now, but please take a look too yourself.

All the other bugs and comments I have fixed and it should work well now. Please take a look and let me know what you think.

kilobyte2007 commented 2 years ago

Another thing I think we should do in this release is some housekeeping - updating the packages where we can, as some of them throw deprecation warnings and have security vulnersbilities:

  1. tippy to 6
  2. deepmerge to 4
  3. autosize to 4
  4. autoprefixer to 10
  5. cssloader to 6
  6. eslint to 8 (same for all eslint packages)
  7. rimraf to 3

tippy might have some breaking changes but others should not require any code changes. What do you think?

JosephusPaye commented 2 years ago

Thanks for making the changes!

The devtool still works even without the flag when running the production build of docs.

That's good to know.

As for the breaking changes - yes, would it be better to document them in the release notes/changelog somewhere?

Yeah, CHANGELOG.md is where we should document the breaking changes.

Also I added a fix for the calendar as it was inheriting some global styles for table from the docs but it was looking differently when using KeenUI in our project as that style was not included in the bundle. Maybe it makes sense to never use global table and td styles in the docs CSS so that we can be sure that the docs reflect everything correctly.

Agreed. We can add a class to apply the docs-specific table styles, and only use it where necessary. You change to the component looks good though, we can keep that.

The UiSelect issue I managed to fix as it seems that both the ui-popover and the ui-select were listening to the outside click event, it seems to work perfectly now, but please take a look too yourself.

Could you say more what the issue was?

Another thing I think we should do in this release is some housekeeping - updating the packages where we can, as some of them throw deprecation warnings and have security vulnersbilities:

Agreed. Though it would be better to that in a separate PR. The Tippy breaking change should be OK if we document it along with the other breaking changes.

kilobyte2007 commented 2 years ago

As for the UiSelect issue - the thing was that both the UiPopover and the UiSelect were listening to an outside click, so when the outside click event was caught in UiSelect - the UiPopover's isOpen returned false as it was already closed by that point so the closeDropdown method was never called inside UiSelect which ultimatelly emits a touch event that was used in the validation example. What I had to do is to disable close-on-external-click for the UiPopover inside the UiSelect as this behavior is controlled at the UiSelect level.

I have also gone ahead and fixed the broken image placeholders (placehold.it moved to via.placeholder.com now).

As for the tables - I think we can just revert to using only .table instead of the table in the docs CSS files so that we don't mix it with the component's actual styles because wherever a table is used the class is present now. Should I include this change in this PR too?

As for the Changelog, should I do this in this PR or will you document that when you release a new version? Here's a breakdown of the breaking changes and bugfixes:

Breaking changes

  1. modelValue should be used on all components where the value prop was used. This includes the following components: UiTextbox, UiSelect, UiCalendar, UiCheckbox, UiCheckboxGroup, UiRadioGroup, UiDatepicker, UiDatepickerCalendar, UiAutocomplete, UiSlider, UiSwitch.
  2. The update:modelValue event should be used wherever the input event was used. This includes the following components: UiTextbox, UiSelect, UiCalendar, UiCheckbox, UiCheckboxGroup, UiRadioGroup, UiDatepicker, UiDatepickerCalendar, UiAutocomplete, UiSlider, UiSwitch, UiFileupload.
  3. The way KeenUI can be used globally has changed.

Fixes and updates

  1. The touch event was fixed for the UiSelect component. It was not fired previously when clicking away from the dropdown.
  2. The UiCalendar styling was fixed as it was inheriting some styles from the docs site.
  3. Fixed the UiIconButton focus containing when used with a dropdown.
  4. UiCloseButton - fixed warning (this.loading check was not needed here)
  5. UiDatepickerCalendar - fixed usage of a year range.

Please let me know what else is needed for me in order for this PR to be merged. Thanks!

kilobyte2007 commented 2 years ago

Another thing. I went ahead a made a small improvement to some components that emit a new value if a previous one was invalid. Previously there were possibilities of events being emitted even though the value has never changed actually (UiCheckbox, UiSwitch) or in the case of UiSelect - it was always emitting an input and a change event even though the value was equal ''.

Please take a look and let me know what you think of it, I'd be happy if we could add this change to this PR as well as it would help solve a lot of issues. Commit - https://github.com/kilobyte2007/Keen-UI/commit/047c211e294df003a211090e16a0645cc309d8a7

JosephusPaye commented 2 years ago

As for the UiSelect issue - the thing was that both the UiPopover and the UiSelect were listening to an outside click, so when the outside click event was caught in UiSelect - the UiPopover's isOpen returned false as it was already closed by that point so the closeDropdown method was never called inside UiSelect which ultimatelly emits a touch event that was used in the validation example. What I had to do is to disable close-on-external-click for the UiPopover inside the UiSelect as this behavior is controlled at the UiSelect level.

Makes sense.

I have also gone ahead and fixed the broken image placeholders (placehold.it moved to via.placeholder.com now).

Great, thanks!

As for the tables - I think we can just revert to using only .table instead of the table in the docs CSS files so that we don't mix it with the component's actual styles because wherever a table is used the class is present now. Should I include this change in this PR too?

Yeah that's a good approach. I think what you've added now (the .table prefix) is OK for this PR.

As for the Changelog, should I do this in this PR or will you document that when you release a new version? Here's a breakdown of the breaking changes and bugfixes:

Thanks for the list of changes. I'll add them to the changelog after we merge this PR.

Please take a look and let me know what you think of it, I'd be happy if we could add this change to this PR as well as it would help solve a lot of issues. Commit - https://github.com/kilobyte2007/Keen-UI/commit/047c211e294df003a211090e16a0645cc309d8a7

Let's do this one in a separate PR.

Please let me know what else is needed for me in order for this PR to be merged. Thanks!

This PR is very close to merging. I've done another review with your latest changes. Once you address the comments we can merge this!

Thanks a lot for your efforts on this.

kilobyte2007 commented 2 years ago

Hey @JosephusPaye, I just implemented the changes. Let me know if there's something else. For the emits proposal - I will create a separate PR once this one is merged. Thanks for taking the time to review everything. When the change is so big it's easy to miss some things even when I try to be very careful :-)

kilobyte2007 commented 2 years ago

Thanks a lot for catching those! Fixed now.

kilobyte2007 commented 2 years ago

No problem at all, happy to move the library further and use it in our projects. ...and sure, I have submitted a few by now, and I will be submitting a few more, mostly very small ones. Let's discuss that in there :-)