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

feat: brand.yml support in shinylive for Python #187

Merged
merged 12 commits into from
Nov 9, 2024

Conversation

gadenbuie
Copy link
Contributor

This PR brings shinylive support for brand.yml with a number of updates:

  1. First, we now bundle libsass in the pyodide distribution. I'm currently building libsass-python to pyodide in GitHub Actions in my fork of libsass-python. It'd be nice to bring the package building into this repo. If that's easy for someone with more experience with pyodide tooling, I'd be happy for the help. Otherwise, I don't think it's worth the side-quest at the moment.

  2. libsass is included in the pyodide distribution but requires shiny[theme] be declared in requirements.txt. This matches how theming works locally and reduces surprises. To make this work, I had to update our dependency installation to support installing package extras.

    Prior to this PR, including shiny[theme] in requirements.txt would only install shiny, not the extra theme requirements. Now, extra requirements are also installed. This isn't directly supported by micropip, so we manually look up the extra dependencies and install them directly.

    An informative message is printed when this happens, e.g.

    Installing libsass for shiny[theme]
    Installing brand-yml for shiny[theme]
    
  3. I added a branded theming example to the examples list. Branded theming uses .yml and .scss files, so I also updated codemirror to include their language support.

  4. Finally, I wanted to link to the brand-yml website from the example description, so I updated the Example React component to accept raw HTML in about.txt, which is sanitized via DOMPurify. We also add target="_blank" to any <a> elements.

@@ -97,7 +97,7 @@
},
"plotnine": {
"name": "plotnine",
"version": "0.0.post20+g64295e1",
"version": "0.0.post1930+g64295e1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this change shouldn't be committed -- I believe your local copy of plotnine is getting post1930 probably because it doesn't have the tags cloned, or something like that. (This is another case where the auto-numbering based on commits since the last tag is difficult to deal with.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh shoot, yeah I meant to do a git add --patch here but got carried away when adding the file to the commit, I'll fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 2c4e1a2

Comment on lines +442 to +443
if f'extra == "{extra}"' in str(r)
or f"extra == '{extra}'" in str(r)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will they always have this exact formatting, with the spaces around ==?

The reason I'm asking is because I don't have a sense of what the items in extras look like. It would be helpful if, up above around where extras.update() is called, there was a comment illustrating what the item looks like, similar to the comment that shows what pkg_name looks like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will they always have this exact formatting, with the spaces around ==?

I think so, or at least if I've found the right documentation it appears that it will: https://packaging.python.org/en/latest/specifications/core-metadata/#provides-extra-multiple-use.

It would be helpful if ... there was a comment illustrating what the item looks like

Maybe you were scanning quickly; there's exactly this comment two lines above, before extra_reqs is defined.

src/hooks/usePyodide.tsx Outdated Show resolved Hide resolved
@@ -0,0 +1,93 @@
Copyright 2021 The Monda Project Authors (https://github.com/googlefonts/mondaFont)
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Commenting here because I can't comment on the .ttf file directly.)
Something to keep in mind for future examples: All of the examples right now are bundled into a single file, examples.json, and that whole file must be loaded before the page starts running an example. So that's a reason to try to example file sizes small. In the future, it might be better to put each example in its own .json file that is loaded on demand. I don't think it's a show stopper here, but it is something to keep in mind in the future when adding large examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I included this in part to test out the full range of features. Having tested it, I'm on the fence. I'd be okay taking it out for simplicity and sizing; or I'd equally be okay with leaving it so that we continue to test it manually before py-shiny releases. It adds about 200kb to examples.json, which is ~400kb before this change and just over 600kb after.

// Sort files, with app related files first, followed by common
// support files, other files in normal sorted order.
const primary = ["app.py", "app.R", "server.R"];
const secondary = ["_brand.yml", "requirements.txt"];
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm understanding correctly, this means that _brand.yml and requirements.txt would come before other files, like utils.py. Maybe we should have an ordering like this?

  1. app.py, app.R, server.R
  2. other .py and .R files
  3. _brand.yml, requirements.txt
  4. everything else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, done in 1f3e436

@@ -0,0 +1,2 @@
Branded Theming
With a custom <a href="https://posit-dev.github.io/brand-yml">brand.yml</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe target="_blank" here?

Also, now that I think about it, I kind of feel like there shouldn't be a link here -- the navigation of the sidebar right now has a simplicity and consistency to it, and adding a link here means that the user has to be somewhat more careful about clicking on the right part of the item to get the example to show, as opposed to opening the brand.yml web page.

So I think it would make sense to put the URL in a comment at the top of the app code, and also perhaps as an active link at the top of the running app, and then change this to regular text (and also undo the changes below to allow rending of the <a> tag).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I can get behind this. It'd be nice to have a better interface for additional content – sort of like the showcase mode – but I agree we should keep this simple for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in cd71c01 and a523d69

@gadenbuie gadenbuie requested a review from wch November 8, 2024 22:50
@gadenbuie
Copy link
Contributor Author

@wch I've addressed feedback (thanks!) and linked to relevant commits in the comments above. The only thing I didn't fully resolve is the inclusion of the font file (#187 (comment))

Copy link
Collaborator

@wch wch left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for addressing the comments! I think the font thing is OK for now -- we'll just want to be mindful of the examples.json size in the future.

@wch wch merged commit 8b313c1 into main Nov 9, 2024
2 checks passed
@wch wch deleted the feat/bundle-libsass-pyodide branch November 9, 2024 00:06
@wch
Copy link
Collaborator

wch commented Nov 9, 2024

@gadenbuie after it gets built and deployed to github.io and you check that it looks right, feel free to move the deploy branch to point to the same commit as main.

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.

2 participants