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

[Vue] convert sites manager single site template/controller to vue component #18562

Merged
merged 675 commits into from Feb 2, 2022

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Jan 3, 2022

Description:

This PR is based off of #18553.

Changes:

  • Fix typing error in SiteSelector.
  • Convert sites-manager-site.controller.js to SiteFields.vue component.
  • Add stores for timezone/currency/site type for use in SitesManager.
  • Created lazyInitSingleton function that constructs a singleton when it is first accessed so we don't end up sending a bunch of ajax requests on page load for stores that may not be used.

image

Review

… reusable function to create vue app and add globals to it
@sgiehl sgiehl modified the milestones: 4.7.0, 4.8.0 Jan 18, 2022
@diosmosis diosmosis added the Needs Review PRs that need a code review label Jan 21, 2022
@diosmosis diosmosis marked this pull request as ready for review January 21, 2022 02:13
@sgiehl
Copy link
Member

sgiehl commented Jan 21, 2022

Here a few issues found while clicking through the UI. Some might possibly be unrelated to the changes here.
Only did some quick testing yet. Maybe something else pops up when I continue the review...

  • When adding a new site the cancel button does not work
  • When searching for a site has exactly 10 results the next button is clickable (even though there aren't any further results)

@diosmosis
Copy link
Member Author

diosmosis commented Jan 22, 2022

When searching for a site has exactly 10 results the next button is clickable (even though there aren't any further results)

This is a problem w/ 4.x-dev. It can be fixed easily but better to wait for these fixes until the next PR since the fix would still be in the angularjs code here.

@diosmosis
Copy link
Member Author

When adding a new site the cancel button does not work

Fixed.

@sgiehl
Copy link
Member

sgiehl commented Jan 31, 2022

@diosmosis Adding a rollup doesn't work with the changes here. The field for selecting the sites isn't displayed without any error. It works on 4.x-dev, so seems to be related to the changes here. Can you check if we can get that running again without changes in the RollUp plugin?

@diosmosis
Copy link
Member Author

@sgiehl should be fixed

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Found some more issues:

  • When adding a new site, it automatically added at the top of the list. But the currency and timezone is always empty for those new sites:
    image
    When editing a site the display values in the list are also not updated.

Besides that it looks fine and should be good to merge, once those issues are fixed.

plugins/SitesManager/vue/src/SiteFields/SiteFields.vue Outdated Show resolved Hide resolved
@diosmosis
Copy link
Member Author

When adding a new site, it automatically added at the top of the list. But the currency and timezone is always empty for those new sites:

This is broken on 4.x-dev too. It should be fixed in the next PR.

Here it seems a & is missing between the url and suffix

Fixed.

@sgiehl sgiehl merged commit 974511b into 4.x-dev Feb 2, 2022
@sgiehl sgiehl deleted the vue-sitesmanager branch February 2, 2022 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants