Skip to content

Conversation

@pferreir
Copy link
Member

@pferreir pferreir commented Aug 9, 2024

I think this is finally ready for a first pair of extra eyes. I still have to go over it a bit more carefully, but having someone else look at it will help find any issues.

Main changes:

  • New events targeting VC rooms and associations. I didn't go as far as replacing the whole plugin inheritance hell with it, but that should be the direction for future plugins;
  • Added a proper VCRoom.delete method which deals with the logic of deleting a whole room and its associations
  • Changed session_updated and session_block_updated to allow for a changes dict to be passed
  • Made split_log_location_changes non-destructive, since object mutation was messing with some tests
  • A few UI changes to use Semantic UI and custom elements instead of legacy CSS when rendering almost all VC-related stuff
  • Session delete operation changed in order to make signal logic more usable
  • Event test fixture: give creator management access by default (as is the case in prod)

Unrelated things which are bundled too:

  • Serialize locators as dicts and add clearer repr
  • Add format_repr to VCRoomEventAssociation

PRs for the CERN and general-purpose plugins will follow.

Copy link
Member

@tomasr8 tomasr8 left a comment

Choose a reason for hiding this comment

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

Left you some comments :) I'm not that familiar with the VC code so some of them might be off base..

@pferreir pferreir force-pushed the vc-zoom-rooms branch 2 times, most recently from 6186ac3 to 6ca6296 Compare September 27, 2024 13:26
@pferreir pferreir force-pushed the vc-zoom-rooms branch 2 times, most recently from 879a74f to 007e455 Compare October 14, 2024 13:12
@pferreir pferreir marked this pull request as ready for review October 14, 2024 13:46
@ThiefMaster
Copy link
Member

image

Might be nice to make the minutes button a bit bigger so they're all the same height?

@pferreir
Copy link
Member Author

image

Might be nice to make the minutes button a bit bigger so they're all the same height?

image

@ThiefMaster
Copy link
Member

Much nicer, even though the font size is pretty tiny... :/

@pferreir
Copy link
Member Author

Much nicer, even though the font size is pretty tiny... :/

Not much we can do, I'm afraid. It's consistent with the other buttons, right? TBH, I felt tempted to just get rid of the text altogether and leave only the icon.

@ThiefMaster
Copy link
Member

Maybe icon-only is not such a bad idea (but keeping visually hidden text for a11y so it's not really just an icon).

@pferreir
Copy link
Member Author

Maybe icon-only is not such a bad idea (but keeping visually hidden text for a11y so it's not really just an icon).

Screencast.from.2024-10-31.12-17-38.webm

@ThiefMaster ThiefMaster added this to the v3.3 milestone Oct 31, 2024
@ThiefMaster ThiefMaster added the build-wheel Build a Python wheel for this PR label Oct 31, 2024
@ThiefMaster ThiefMaster merged commit 9485bf9 into indico:master Nov 4, 2024
@ThiefMaster ThiefMaster deleted the vc-zoom-rooms branch November 4, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build-wheel Build a Python wheel for this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants