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

Add include_javascript(), include_css(), and include_html() #127

Merged
merged 16 commits into from
May 17, 2023
Merged

Conversation

cpsievert
Copy link
Collaborator

@cpsievert cpsievert commented Apr 19, 2022

Currently requires posit-dev/py-htmltools#32 (since, if we want to return a Tag, we need to know the final src/href).

TODO:

  • Add a include_markdown() that utilizes markdown()?
  • Probably split up include_html() into include_html() and include_html_frame()?
  • Finish include_html()/include_html_frame() examples

@cpsievert cpsievert requested a review from wch April 19, 2022 15:14
@cpsievert cpsievert marked this pull request as ready for review April 19, 2022 15:14
@Polkas
Copy link

Polkas commented Aug 11, 2022

linked to posit-dev/py-htmltools#42
Please remember there is no htmltools::singleton now in the python version. In my opinion we need such especially when connected with this PR functions will be introduced.

shiny/ui/_include_helpers.py Outdated Show resolved Hide resolved
shiny/examples/include_css/app.py Outdated Show resolved Hide resolved
shiny/examples/include_html/app.py Outdated Show resolved Hide resolved
shiny/examples/include_javascript/app.py Outdated Show resolved Hide resolved
shiny/examples/include_html/app.py Outdated Show resolved Hide resolved
shiny/ui/_include_helpers.py Outdated Show resolved Hide resolved
shiny/ui/_include_helpers.py Outdated Show resolved Hide resolved
shiny/ui/_include_helpers.py Outdated Show resolved Hide resolved
shiny/ui/_include_helpers.py Outdated Show resolved Hide resolved
shiny/ui/_include_helpers.py Outdated Show resolved Hide resolved
shiny/ui/_include_helpers.py Show resolved Hide resolved
shiny/ui/_include_helpers.py Outdated Show resolved Hide resolved
shiny/ui/_include_helpers.py Outdated Show resolved Hide resolved
shiny/ui/_include_helpers.py Outdated Show resolved Hide resolved
shiny/examples/include_css/css/styles.css Outdated Show resolved Hide resolved
shiny/ui/_include_helpers.py Outdated Show resolved Hide resolved
shiny/ui/_include_helpers.py Outdated Show resolved Hide resolved
shiny/ui/_include_helpers.py Outdated Show resolved Hide resolved
shiny/ui/_include_helpers.py Outdated Show resolved Hide resolved
shiny/ui/_include_helpers.py Outdated Show resolved Hide resolved
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.

I had some more thoughts about path handling and the user experience. What happens if someone writes this reasonable-looking code?

app_ui = ui.page_fluid(
    ui.include_css("css/my_styles.css"),
    ...
)

It's a relative path, but we can make any assumptions about which directory is the current working directory. This could result in a not-very-clear error that will leave the user scratching their head.

So we may want to test that the path is an absolute path at the top of include_js and include_css, with something roughly like:

path = Path(path)
if not path.is_absolute():
    raise RuntimeError('include_js() requires an absolute path. Please use
  include_js(Path(__file__)).parent / "myfile.css"') 

(But with a better error message)

The other comments I have about Path are related to this. When the PR was originally created, we mostly used os.path, but since then we generally switched to pathlib.Path.

shiny/ui/_include_helpers.py Outdated Show resolved Hide resolved
shiny/examples/include_css/app.py Outdated Show resolved Hide resolved
shiny/examples/include_javascript/app.py Outdated Show resolved Hide resolved
@gshotwell
Copy link
Contributor

@wch It seems like Path("css/my_css.css").absolute() will always return the absolute path if it can find the file. Do we really need that check?

@wch
Copy link
Collaborator

wch commented May 16, 2023

The issue is that if you start the app from a different directory, then the path will be relative to that directory. For example:

[winston@MBP16:~]$ tree temp
temp
├── app.py
├── css
    └── my_css.css

[winston@MBP16:~]$ pwd
/Users/winston

[winston@MBP16:~]$ cat temp/app.py
from pathlib import Path
print(Path("css/my_css.css").absolute())
...

[winston@MBP16:~]$ shiny run temp/app.py
/Users/winston/css/my_css.css

We would want it the printed path to be /Users/winston/temp/css/my_css.css, not /Users/winston/css/my_css.css.

As far as I know, the only reasonable way to do that is to have the user provide __file__ somwhere.

@gshotwell
Copy link
Contributor

Oh I see, so we really want to be checking if the file exists? I'm not sure we want to enforce the __file__ pattern because they might be using css which is hosted outside of the app folder right? Like if you had a single css file which was used by multiple apps. This isn't a great pattern but I don't think it should provoke an error.

What about this:

def check_path(path: Path) -> Path:
    path = Path(path)
    if not path.exists():
        err = f'{path.name} does not exist. Typically you should refer to this file with 'Path(__file__) / {path.name}'")'
        raise RuntimeError(err)
    return path
    ```

@wch
Copy link
Collaborator

wch commented May 16, 2023

Oh I see, so we really want to be checking if the file exists?

Yes, that makes sense.

I'm not sure we want to enforce the __file__ pattern because they might be using css which is hosted outside of the app folder right? Like if you had a single css file which was used by multiple apps. This isn't a great pattern but I don't think it should provoke an error.

Good point. I think we should use the __file__ pattern in our examples, though.

What about this:

def check_path(path: Path) -> Path:
    path = Path(path)
    if not path.exists():
        err = f'{path.name} does not exist. Typically you should refer to this file with 'Path(__file__) / {path.name}'")'
        raise RuntimeError(err)
    return path
    ```

That sounds good to me.

@wch wch merged commit be137b0 into main May 17, 2023
@wch wch deleted the include-helpers branch May 17, 2023 18:50
@gshotwell gshotwell mentioned this pull request May 23, 2023
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