Skip to content

Initial Qrcode implementation based on JT's interregnum work - #7718

Merged
hatton merged 4 commits into
masterfrom
qrcode
Mar 11, 2026
Merged

Initial Qrcode implementation based on JT's interregnum work#7718
hatton merged 4 commits into
masterfrom
qrcode

Conversation

@StephenMcConnel

@StephenMcConnel StephenMcConnel commented Mar 4, 2026

Copy link
Copy Markdown
Contributor

Open with Devin

This change is Reviewable

@StephenMcConnel
StephenMcConnel marked this pull request as draft March 4, 2026 00:03
devin-ai-integration[bot]

This comment was marked as resolved.

@StephenMcConnel StephenMcConnel left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@StephenMcConnel resolved 5 discussions.
Reviewable status: 0 of 19 files reviewed, all discussions resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

@StephenMcConnel
StephenMcConnel force-pushed the qrcode branch 2 times, most recently from 8e4d693 to 7dc2c0f Compare March 5, 2026 00:23
devin-ai-integration[bot]

This comment was marked as resolved.

@StephenMcConnel
StephenMcConnel marked this pull request as ready for review March 5, 2026 01:09
Comment thread src/content/bookLayout/basePage.less Outdated
Comment thread src/content/branding/Default/made-with-bloom-badge.svg
Comment thread src/content/branding/Default/branding.less

@StephenMcConnel StephenMcConnel left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@StephenMcConnel made 3 comments.
Reviewable status: 0 of 19 files reviewed, 3 unresolved discussions (waiting on andrew-polk and hatton).

Comment thread src/content/bookLayout/basePage.less Outdated
Comment thread src/content/branding/Default/branding.less
Comment thread src/content/branding/Default/made-with-bloom-badge.svg
devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

@StephenMcConnel
StephenMcConnel force-pushed the qrcode branch 2 times, most recently from d7346d1 to 17e2294 Compare March 9, 2026 19:42
devin-ai-integration[bot]

This comment was marked as resolved.

@StephenMcConnel
StephenMcConnel force-pushed the qrcode branch 2 times, most recently from 089bac1 to fc4506d Compare March 9, 2026 20:30
devin-ai-integration[bot]

This comment was marked as resolved.

@hatton hatton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@hatton partially reviewed 13 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: 13 of 22 files reviewed, 4 unresolved discussions (waiting on andrew-polk and StephenMcConnel).


src/BloomExe/Book/Book.cs line 5346 at r11 (raw file):

        {
            Storage.UpdateSupportFiles();
            BookStorage.UpdateQrCode(

Looking at the parameters here: is it intentional that we are giving it CollectionSettings but also giving it things that are derived from that same object ( CollectionSettings.ShowBlorgLanguageQrCode,
Language1Tag,)?

E.g. two options:

  1. just give it CollectionSettings, don't require the Book.cs to know what it needs
  2. Give it only what it needs: instead of CollectionSettings, give it CollectionSettings.BadgeQrCodeLabelLocalized

src/content/branding/branding-base.less line 95 at r11 (raw file):

    }
    .bloom-lang-on-blorg {
        font-size: smaller;

Just checking: is smaller really what you want? That makes it relative to some other declaration in a way that I don't know... is that good or bad?

@hatton hatton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@hatton resolved 1 discussion.
Reviewable status: 13 of 22 files reviewed, 3 unresolved discussions (waiting on andrew-polk and StephenMcConnel).

@StephenMcConnel StephenMcConnel left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@StephenMcConnel made 2 comments.
Reviewable status: 13 of 22 files reviewed, 3 unresolved discussions (waiting on andrew-polk and hatton).


src/BloomExe/Book/Book.cs line 5346 at r11 (raw file):

Previously, hatton (John Hatton) wrote…

Looking at the parameters here: is it intentional that we are giving it CollectionSettings but also giving it things that are derived from that same object ( CollectionSettings.ShowBlorgLanguageQrCode,
Language1Tag,)?

E.g. two options:

  1. just give it CollectionSettings, don't require the Book.cs to know what it needs
  2. Give it only what it needs: instead of CollectionSettings, give it CollectionSettings.BadgeQrCodeLabelLocalized

Done (#2)


src/content/branding/branding-base.less line 95 at r11 (raw file):

Previously, hatton (John Hatton) wrote…

Just checking: is smaller really what you want? That makes it relative to some other declaration in a way that I don't know... is that good or bad?

It's relative to the inherited font-size of 1em (which was essentially 10pt). I've changed it to 9pt (which is very slightly bigger than what the "smaller" keyword produced).

@StephenMcConnel StephenMcConnel left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@StephenMcConnel made 1 comment.
Reviewable status: 10 of 22 files reviewed, 3 unresolved discussions (waiting on andrew-polk and hatton).


src/BloomExe/Book/Book.cs line 5346 at r11 (raw file):

Previously, StephenMcConnel (Steve McConnel) wrote…

Done (#2)

Reviewable did something strange with my attempt to indicate that I used option number 2.

@hatton hatton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@hatton reviewed 3 files and resolved 3 discussions.
Reviewable status: 13 of 22 files reviewed, all discussions resolved (waiting on andrew-polk).

@hatton
hatton merged commit 310a27a into master Mar 11, 2026
1 of 2 checks passed
@hatton
hatton deleted the qrcode branch March 11, 2026 20:08
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.

4 participants