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

Update sort and filter at meeting switch #2947

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

reiterl
Copy link
Member

@reiterl reiterl commented Oct 25, 2023

Closes #2870

Small cleanup

@reiterl reiterl added the bug label Oct 25, 2023
@reiterl reiterl added this to the 4.1 milestone Oct 25, 2023
@reiterl
Copy link
Member Author

reiterl commented Oct 25, 2023

I didn't include changes the the ListSearchService here, because this service works differently than filter and sort service.

@bastianjoel bastianjoel removed their assignment Oct 27, 2023
Copy link
Member

@Elblinator Elblinator left a comment

Choose a reason for hiding this comment

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

function

@Elblinator Elblinator merged commit 2c0b95f into OpenSlides:main Oct 27, 2023
5 checks passed
@reiterl reiterl deleted the 2870-filter-sort-meeting-switch branch October 27, 2023 11:16
@@ -177,6 +179,8 @@ export abstract class BaseSortListService<V extends BaseViewModel>

private initializationCount = 0;

private activeMeetingIdService = inject(ActiveMeetingIdService);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be injected via the constructor like all other services?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had problems with the use of constructor injection here, as I need to update 5+ super calls then. I talked to @bastianjoel about this and he said I can use this method here.

Copy link
Member

Choose a reason for hiding this comment

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

inject was introduced in Angular 14. I assume that this was not done before because the feature was not available when these classes were written.
In my opinion, placing service injections in the constructor within base classes (or any other classes that will be extended) makes these classes unnecessarily harder to maintain as you always need to update all references when adding or removing a service the class is using.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree! I just asked for consistency reasons. I'm all for switching all classes to the new injector style, if possible - the long constructor chains have always bugged me immensly. Is there maybe an automatic migration tool to get rid of these?

m-schieder added a commit that referenced this pull request Nov 9, 2023
* commit '2904fb2ac5d9b8a39fff48e8ea04353077e0a3dc': (26 commits)
  Make user option title load - alternative (#2995)
  Fix safari chyron font (#2990)
  Move motion block recommendation text after icon (#2979)
  Global search style clean-up (#2977)
  Add tests to openslides-status service (#2960)
  Correcting translations (#2980)
  Fix line numbers at change context remainder (#2970) (#2978)
  Update los contribution count (#2968)
  Mediafile support recursive folder download (#2866)
  Add validators to email and default_vote_weight in some dialog (#2911)
  Fix a problem in the poll service (#2963)
  Fix version history-link (#2967)
  Add missing sorting for filter properies in filter list (#2953)
  Update way to check for agenda-item-list (#2965)
  Use gramatically correct email snackbar messages (#2954)
  Update agenda projection (#2964)
  Change duration of request of speakers to hours:mins (#2948)
  Update sort and filter at meeting switch (#2947)
  Fix missing star icon in motion-block (#2952)
  Bump crypto-js from 4.1.1 to 4.2.0 in /client (#2950)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Motions: Filter stays active after meeting switch
4 participants