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

[DOCS] fix & event-handlers #852

Closed
wants to merge 1 commit into from
Closed

Conversation

dj-fiorex
Copy link
Contributor

Fix markdown
Add event-handlers.md

Add event-handlers.md
@marimeireles
Copy link
Member

Wow!
Great work! 🎉
Thank you so much for these ☺️

@marimeireles
Copy link
Member

closes #835

Copy link
Contributor

@antocuni antocuni left a comment

Choose a reason for hiding this comment

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

Thank you for this PR @dj-fiorex!
I have added a small suggestion but overall it looks very good.
(And I seem to understand that you are italian as well.... benvenuto 🇮🇹 )

Two notes though:

  • we are thinking about a heavy refactoring of py events, see e.g. Behaviour of py-*: how to pass event #835. But this should not prevent this PR to be merged because it is a very good description of the current status, so good job.
  • We cannot merge the PR as is because there is a conflict on passing-objects.md, but hopefully it should be easy to fix.

## Subscribe to event with `py-*` attributes

You can subscribe to an event as you would with Javascript, by adding a `py-*` attribute to an HTML element. The value of the attribute should be a Python function.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is not technically correct. The value of the attribute can be any arbitrary python code which will be executed, not necessarily a function call. This is the relevant code:

script.addEventListener('load', () => {
void this.afterRuntimeLoad(runtime);
});

that said, it is true that the de-facto standard way to use them is to call a function (to which you can pass arbitrary arguments).

So you can maybe say something like this:

The value of the attribute contains python code which will be executed when the event is fired. A very common pattern is to call a function which does further work, for example: ...

(but feel free to adjust the wording as you like)

</py-script>
```

## JavaScript to PyScript
Copy link
Member

Choose a reason for hiding this comment

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

This section and the following one seem to be identical to the text in passing-objects.md. We probably want to avoid duplicating documentation across multiple locations - it's just more chances for the documents to get out of sync with each other and the codebase.

I'd agree that JS/Python object interaction is relevant here though - I think it'd be good to rework this to refer to the existing object-passing documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is identical because the section "passing-objects" is a very broad argument, but I think it's important to have this examples here

Copy link
Member

Choose a reason for hiding this comment

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

I agree that they're related, but I don't think it merits having duplicate content - almost all of PyScript touches both JavaScript and Python in some way, but duplicating examples across several pages is probably not the clearest way to communicate that information.

Perhaps adding a note to the start of the "Subscribe to Events with AddEventListener" section like:

The following uses the from js import ... syntax to import references to JavaScript directly into Python. See [passing objects] for more details.

So that the reader is primed to look for that info and knows where to find it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Jeff: let's avoid duplication and put cross-reference links instead.
It is easier for users to read, and it will also make our life easier whenever we change anything and we need to update the docs

@JeffersGlass JeffersGlass added the tag: docs Related to the documentation label Nov 13, 2022
@JeffersGlass
Copy link
Member

Hi @dj-fiorex! Just wanted to check in about this PR - it's really great work, I think it could just use some de-duplication and it'd be ready to merge.

@dj-fiorex
Copy link
Contributor Author

Hello sorry for the delay, I will fix this next week!

@marimeireles
Copy link
Member

closing this one bc i wasn't allowed to push to @dj-fiorex's remote!
Thanks for the work!

@marimeireles marimeireles mentioned this pull request Mar 23, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag: docs Related to the documentation
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants