-
-
Notifications
You must be signed in to change notification settings - Fork 553
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
XWIKI-18998: Replace the Livetable of the System Filters of the Notifications Administration with a Live Data macro #2827
Conversation
25d6509
to
b3722f9
Compare
5277409
to
c074b81
Compare
@@ -108,11 +108,11 @@ | |||
#set ($objectNumber = $!obj.reference.objectNumber) | |||
#end | |||
#if ($checked) | |||
#set ($checked = 'checked = "checked"') | |||
#set ($checkedAttr = 'checked = "checked"') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm honestly wondering if it wouldn't be better for the long term maintenance to get rid of that, and provide a dedicated Java source for the LD. Feels like it would be easier to test / maintain, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept that one because I considered it could be used by extensions. But yes, might not be a bad idea to provide a clean source and to move that whole one to legacy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, actually there is 3 other livetable using this results page, so we'll need to migrate them as well before being able to depracate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept that one because I considered it could be used by extensions.
I honestly doubt it but yes it makes sense to be conservative.
Well, actually there is 3 other livetable using this results page, so we'll need to migrate them as well before being able to depracate.
I was thinking about doing that for the custom notification filters: I don't recall but I guess it's same page used for those too. Actually I'm surprised: you do have filtering / sorting with that page result? Or the filtering / sorting only work on frontend with the live data here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think it's ok maybe we can do the move by steps:
- merge that,
- create an improvment ticket to have dedicated source for LD for those
- open a vote to get rid of this LT results (would avoid having to maintain it on the long run)
- migrate other LT to LD using the source from 2 and migrate again that one with the source too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, I'm taking a close look at the migration, and it's not "too" difficult (modulo the random bumps you can get when migrating messy Velocity script), but this PR is already quite large, so I'm not against delaying this to another step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I'm surprised: you do have filtering / sorting with that page result? Or the filtering / sorting only work on frontend with the live data here?
No, I just realized the mistake when looking at the migration to a LD source. Fixing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be better with 75fd7ef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2. create an improvment ticket to have dedicated source for LD for those
c074b81
to
75fd7ef
Compare
…ications Administration with a Live Data macro
…ications Administration with a Live Data macro
…ications Administration with a Live Data macro
…ications Administration with a Live Data macro Adding missing properties for filtering and sorting deactivation.
75fd7ef
to
bce83ab
Compare
Jira URL
https://jira.xwiki.org/browse/XWIKI-18998
Changes
Description
Bonus: better handling of the toggles on error
Clarifications
DisplayerToggle
Vue component for the display of the togglers in the newxwiki-platform-notifications-webjar
(second webjar + npm module afterxwiki-platform-livedata-webjar
).src/main/vue
instead having the config in a separate place, making the code and the build more difficult to understandxwiki-platform-livedata-webjar
now exposesdisplayerMixin
,BaseDisplayer
,XWikiIcon
, andXWikiLivedata
(instead ofXWikiLivedata
) because they are required byDisplayerToggle
)XWikiIcon
, so thatDisplayerToggle
can know when the component is initialized.Screenshots & Video
Before
After
Error handling
Note how the toggle is not switched on error. Previously, the toggle state would change even in case of error.
Screencast.from.26-01-2024.16.25.25.webm
Executed Tests
Expected merging strategy