[FEATURE] Data,Refinery: add QR code support#11391
[FEATURE] Data,Refinery: add QR code support#11391lscharmer merged 2 commits intoILIAS-eLearning:trunkfrom
Conversation
3de0fa2 to
0e88c35
Compare
|
Assessment: The General Information:
Type of dependency:
Usage:
Reasoning:
Maintenance:
Links:
Alternatives: There is only one other widely used library, which does not rely on the
|
|
Hi @thibsy, here are my thoughts on this: In terms of accessibility: Kind regards and thank you again for this PR |
|
Hi @lscharmer, thx for the quick feedback! I think its not quite possible to pass around the information which makes up the QR-code, since this reaches into the dependency too far. We could only introduce a wrapper so we can limit what kind of data can be transformed into a QR-code result, but this wouldn't add much value. I'm therefore beginning to think we should just drop the DTO and only work in transformations. How does that sound to you? Give me a quick ping if we should discuss this on Discord. Also thanks for your thoughts on the accessibility. You're totally right – this is exactly what my UI interface is starting to look like (encapsulate different formats of which QR code is only one). Kind regards, |
|
Hi @lscharmer, As quickly discussed on Discord, I have removed the QR code DTO for the reasons above and implemented the abstraction primarily based on transformations. This PR now introduces:
Since the Looking forward to a more concrete review now =). Thx and kind regards, |
|
P.S. once the changes are approved I will squash the commits appropriately, so we can rebase and integrate one commit for the abstraction (or two for each component if you prefer) and one for the dependency. |
lscharmer
left a comment
There was a problem hiding this comment.
Hi @thibsy,
thanks updating this PR. The structure looks good to me, there is just one naming issue left.
Best regards
@lscharmer
|
Hi @lscharmer, Thx for the feedback, I will incorporate the changes sometime soon. Since everything else seems to be in order, I will add the JF label now to request the dependency. Hope thats OK. Kind regards, |
|
Jour Fixe, 27 APR 2026: We highly appreciate the suggested library and accept this dependency for trunk. |
* Add `ILIAS\Data\SVG` class for wrapping raw SVG images as string. * Add `ILIAS\Data\QR\ErrorCorrectionLevel` for standardised error correction levels. * Add `ILIAS\Refinery\URI\ToSvgQrCodeTransformation` to convert string URI into QR code. * Add `ILIAS\Refinery\URI\FromSvgTransformation` to convert SVG into a URI (as string). * Add unit tests covering the functionality above.
9fe4ef9 to
b96e502
Compare
|
Hi @lscharmer, I have incorporated the requested changes and squashed everything in two commits, one for the feature in data and refinery, the other for the dependency. This should be ready to go now. Best, |
|
I will add the |
Hi @mjansenDatabay,
This is the first iteration of my QR code abstraction for ILIAS, which I plan on using inside the UI framework to convert
ILIAS\Data\URIobjects into a QR code which can be embedded inline as a data URI.I haven't worked out the exact details of the UI interface yet, so there may be changes in the meantime, but I'd still like to hear your 5 cents about this – it doesn't need to be an in-depth code review just yet. I will also wait with the JF label until you have approved of the details, although I will post the formal request in here some time soon already.
What I have worked out:
What I haven't worked out (yet):
Looking forward to your thoughts.
Thx and kind regards,
@thibsy