Skip to content

Add link to graphics #54#102

Open
KimCM wants to merge 3 commits intoDARC-e-V:mainfrom
KimCM:feat/54_increase_graphics_by_clicking_on_them
Open

Add link to graphics #54#102
KimCM wants to merge 3 commits intoDARC-e-V:mainfrom
KimCM:feat/54_increase_graphics_by_clicking_on_them

Conversation

@KimCM
Copy link
Copy Markdown
Contributor

@KimCM KimCM commented Feb 22, 2026

Clicking on an image or drawing opens it in the current browser tab.

Clicking on an image or drawing opens it in the current browser tab.
@KimCM
Copy link
Copy Markdown
Contributor Author

KimCM commented Feb 22, 2026

It should be decided whether the requirement applies A) only to photos or B) to photos AND drawings.

The proposed change implements case B). Please leave a review note if case A) is preferred.

Furthermore, please be aware that this change has an impact on the website AND the slides.

@larsweiler
Copy link
Copy Markdown
Collaborator

This might fail with the slides which use the same renderer for pictures and images.

I can’t test it right now, but this needs to be checked. Especially if you can return to the correct slide when you use your browser’s return button.

@KimCM
Copy link
Copy Markdown
Contributor Author

KimCM commented Feb 22, 2026

with this pr, a photo or picture in a slide is clickable. with a click, the browser tab change to the image. After that, I am able to return with the secondary mouse button, or, with the browser's back button.

BUT: When I activate the speakers view with key s and klick the image there, the content is replaced with the image and I don't find a way back to the "normal" speakers view. This PR should therefore probably not be approved.

@KimCM
Copy link
Copy Markdown
Contributor Author

KimCM commented Feb 22, 2026

Since this is my first time working with Python, I'm not sure what an idiomatic solution would look like.

I am somewhat reluctant to duplicate the render_photo_helper and render_picture_helper methods in fifty_ohm_html_slide_renderer.py, as they would then appear four times in the source code.

Therefore, I suggest this refactoring so that I can implement the creation of the “empty” link in fifty_ohm_html_slide_renderer. Or am I thinking too much in “Java” and should I duplicate the helper methods after all?

In my opinion, there is no rush to approve this PR. I would like to use this as an opportunity to better understand your approach and preferences and adapt accordingly.

@tgrelka tgrelka self-requested a review February 22, 2026 21:55
@tgrelka
Copy link
Copy Markdown
Member

tgrelka commented Feb 22, 2026

@KimCM Thank you for your contribution! A couple of comments:

  1. I like the idea of unifying how figures are rendered! I would prefer if that also extended to the parser, completly unifying the process. This should happen in a separate PR, however. After that is merged, we can return to this feature. Feel free to hit me up on matrix to discuss this. 😄
  2. I'd suggest adding a target="_blank" to the anchor, so images open in a new tab instead of the current one.
  3. From my point of view, there are two options regarding the slides issue.
    1. We could use a class attribute like link_figures = true / false and a guard inside the function for figures, only wrapping the element in a link if the attribute is truthy.

    2. We couly hijack the picture_handler (and image_handler) external function, which currently searches for the requested file, copies it to the correct path and returns an alt-text, if found.

      I already wanted to change the behaviour of this method, as I think it should return both the alt-text, and the entire file path of the asset so the renderer doesn‘t need to know about the location. It could also return a third value, a link. If that is not None, the figure will include a link, otherwise it will be a plain, unclickable image. That way, the behaviour can be controlled externally.

  4. I want to get rid of the helper-type methods in the renderer, as they add a lot of clutter. The way mistletoe works is by using introspection to finde a render_<token_name> function for each token the parser spits out. To avoid problems and especially confusion with additional methods, I‘d prefer if we didn‘t introduce new ones. This would be adressed with point (1), as the two functions would condense into one without needing a helper for shared code.
  5. At some point, we want to implement a proper lightbox (see Increase Graphics by Clicking on Them #54). I‘ve mostly been using a right click with Open image in new tab, which this PR seems to implement by just clicking on the image instead. I think that’s fine as a temporary solution, but it’s likely this will be removed or changed in the future. If we implement lightboxes, we will probably have two completely different render functions for slides and normal HTML, as the lightboxes will require a different markup.
  6. If you make a change, please update and expand the tests so Pytest runs without errors, and includes the new functionality.

I will update the README and add a proper contribution guide in the next few days, which should help new contributors find their way around this repository. I also will start documenting the architecture.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants