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

[binary-search-tree] Four SVG graphs #2285

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

habere-et-dispertire
Copy link
Contributor

No description provided.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2023

Hello. Thanks for opening a PR on Exercism. We are currently in a phase of our journey where we have paused community contributions to allow us to take a breather and redesign our community model. You can learn more in this blog post. As such, all issues and PRs in this repository are being automatically closed.

That doesn't mean we're not interested in your ideas, or that if you're stuck on something we don't want to help. The best place to discuss things is with our community on the Exercism Community Forum. You can use this link to copy this into a new topic there.


Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

@github-actions github-actions bot closed this Jun 7, 2023
@habere-et-dispertire habere-et-dispertire changed the title Added four SVG graphs [binary-search-tree] Four SVG graphs Jun 7, 2023
@iHiD iHiD reopened this Jun 7, 2023
Co-authored-by: Jeremy Walker <jez.walker@gmail.com>
@habere-et-dispertire habere-et-dispertire marked this pull request as ready for review June 7, 2023 15:35
@habere-et-dispertire habere-et-dispertire requested a review from a team as a code owner June 7, 2023 15:35
@SleeplessByte
Copy link
Member

Are these image urls auto-signed @iHiD because right now they don't work.

@habere-et-dispertire
Copy link
Contributor Author

I think [binary-search-tree] Four SVG graphs #14 is awaiting consideration and blocking.

@habere-et-dispertire
Copy link
Contributor Author

CC @exercism/reviewers

@MatthijsBlom
Copy link
Contributor

This replaces ASCII art with links. Is this intentional? I expected images instead of links.

@habere-et-dispertire
Copy link
Contributor Author

Well spottted @MatthijsBlom ! I have moved them inline. 👍

GitHub Preview ( and Gmail ) will still not show them due to site-wide SVG scrubbing due to the potential of JavaScript embedding within SVG. A separation of concern between data and executable seems difficult to maintain : see the historical sagas with Microsoft Word Documents, and also PDFs.

An interesting related read on SVG delineation at mime-type.

@MatthijsBlom
Copy link
Contributor

GitHub Preview ( and Gmail ) will still not show them

They do show up for me:

screenshot

@habere-et-dispertire
Copy link
Contributor Author

Thanks -- they didn't show up in dark mode as the background is transparent and the image was not inverted with dark mode. ( They didn't work at all in the past. )

@MatthijsBlom
Copy link
Contributor

The image not sticking out against dark background sounds like a potential problem for the website as well.

@habere-et-dispertire
Copy link
Contributor Author

Happy to adjust. Are image backgrounds meant to be a solid colour or transparent ? I don't see any documentation for exercism-images .

@MatthijsBlom
Copy link
Contributor

Is the page CSS inaccessible to the SVG? It seems near-ideal to have these images have the same color as the surrounding text.

Next best thing, I guess, would be to have the drawing color be the background inverted, or something like that.

@habere-et-dispertire
Copy link
Contributor Author

Is the page CSS inaccessible to the SVG?

No and that's why I set the background transparent. You only have a single resource then which can be automatically adjusted on light/dark mode preference.

@dem4ron : Does an Exercism CSS class exist for labelling an image as suitable for inversion in dark mode ? It seems they are not all inverted automatically in dark mode since some are not suitable for inversion.

@habere-et-dispertire
Copy link
Contributor Author

So it seems the transparent background is correctly inverted on change of light/dark theme. It is the foreground SVG that is not inverted. If our markdown flavour supports pandoc attribute syntax, we could label the image as fully invertible :

![alt text](the.svg){.invertible}

A compromise approach is to make the stroke on the image a middle gray which should retain enough contrast with the background switching but the foreground not.

@ErikSchierboom
Copy link
Member

If our markdown flavour supports pandoc attribute synta

We don't support that IIRC

@ErikSchierboom
Copy link
Member

I wonder if we should keep some ASCII text. Might be better for accessibility.

@iHiD
Copy link
Member

iHiD commented Aug 11, 2023

Yeah, I don't think we want to lose the ascii text in the local (CLI) instructions. But it would be nice to have them on the website. I don't know how we do that :)

@habere-et-dispertire
Copy link
Contributor Author

Can we put HTML into our Markdown ?

I couldn't seem to get an image fallback that is not an image ( ie ASCII Art ).

The closest I could get to a negotiated content/semantic approach was to hide the ASCII Art inside of the details of a figcaption . This seems to give the shell the ASCII Art, while not cluttering the online version with two representations ( unless you ask for it ).

@ErikSchierboom
Copy link
Member

Most of the html tags would be escaped, so that'll not work I think

@MatthijsBlom
Copy link
Contributor

A few days ago I came across Typograms. Might be a cool idea, but perhaps not easy to implement.

@habere-et-dispertire
Copy link
Contributor Author

Interesting @MatthijsBlom -- thanks ! 😀

Mermaid leans in a more declarative and semantic way if we're looking towards that approach.

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.

5 participants