-
Notifications
You must be signed in to change notification settings - Fork 3
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
Implement support for Overlays #8
Conversation
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.
Thanks! I have a suggestion for improving the implementation, but let's merge and iterate!
class BaseOverlay(DOMWidget): | ||
_model_name = Unicode('BaseOverlayModel').tag(sync=True) | ||
_model_module = Unicode(module_name).tag(sync=True) | ||
_model_module_version = Unicode(module_version).tag(sync=True) | ||
_view_name = Unicode('BaseOverlayView').tag(sync=True) | ||
_view_module = Unicode(module_name).tag(sync=True) | ||
_view_module_version = Unicode(module_version).tag(sync=True) | ||
overlay_type = Unicode().tag(sync=True) | ||
position = List([0, 0]).tag(sync=True) | ||
|
||
class ImageOverlay (BaseOverlay): | ||
overlay_type = Unicode('image').tag(sync=True) | ||
image_url = Unicode('').tag(sync=True) | ||
|
||
class VideoOverlay (BaseOverlay): | ||
overlay_type = Unicode('video').tag(sync=True) | ||
video_url = Unicode('').tag(sync=True) | ||
|
||
class PopupOverlay (BaseOverlay): | ||
overlay_type = Unicode('popup').tag(sync=True) | ||
popup_content = Unicode('').tag(sync=True) |
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.
This approach may be fine for now, but it does not scale well if you have many types of overlays. The BaseOverlayView
implementation front-end side becomes too complicated with a big if
condition and many different methods depending on the type.
It would be nicer to have one widget implementation per overlay type:
class BaseOverlay(DOMWidget): | |
_model_name = Unicode('BaseOverlayModel').tag(sync=True) | |
_model_module = Unicode(module_name).tag(sync=True) | |
_model_module_version = Unicode(module_version).tag(sync=True) | |
_view_name = Unicode('BaseOverlayView').tag(sync=True) | |
_view_module = Unicode(module_name).tag(sync=True) | |
_view_module_version = Unicode(module_version).tag(sync=True) | |
overlay_type = Unicode().tag(sync=True) | |
position = List([0, 0]).tag(sync=True) | |
class ImageOverlay (BaseOverlay): | |
overlay_type = Unicode('image').tag(sync=True) | |
image_url = Unicode('').tag(sync=True) | |
class VideoOverlay (BaseOverlay): | |
overlay_type = Unicode('video').tag(sync=True) | |
video_url = Unicode('').tag(sync=True) | |
class PopupOverlay (BaseOverlay): | |
overlay_type = Unicode('popup').tag(sync=True) | |
popup_content = Unicode('').tag(sync=True) | |
class BaseOverlay(DOMWidget): | |
_model_module = Unicode(module_name).tag(sync=True) | |
_model_module_version = Unicode(module_version).tag(sync=True) | |
_view_module = Unicode(module_name).tag(sync=True) | |
_view_module_version = Unicode(module_version).tag(sync=True) | |
overlay_type = Unicode().tag(sync=True) | |
position = List([0, 0]).tag(sync=True) | |
class ImageOverlay (BaseOverlay): | |
_view_name = Unicode('ImageOverlayView').tag(sync=True) | |
_model_name = Unicode('ImageOverlayModel').tag(sync=True) | |
overlay_type = Unicode('image').tag(sync=True) | |
image_url = Unicode('').tag(sync=True) | |
class VideoOverlay (BaseOverlay): | |
_view_name = Unicode('VideoOverlayView').tag(sync=True) | |
_model_name = Unicode('VideoOverlayModel').tag(sync=True) | |
overlay_type = Unicode('video').tag(sync=True) | |
video_url = Unicode('').tag(sync=True) | |
class PopupOverlay (BaseOverlay): | |
_view_name = Unicode('PopupOverlayView').tag(sync=True) | |
_model_name = Unicode('PopupOverlayModel').tag(sync=True) | |
overlay_type = Unicode('popup').tag(sync=True) | |
popup_content = Unicode('').tag(sync=True) |
This means implementing an ImageOverlayView
, ViewOverlayView
and PopupOverlayView
front-end side.
Let's keep this comment for a future PR and merge.
No description provided.