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][LiveComponent] Add very simple download files docs #2513

Open
wants to merge 7 commits into
base: 2.x
Choose a base branch
from

Conversation

zefyx
Copy link

@zefyx zefyx commented Jan 20, 2025

Q A
Bug fix? no
New feature? no
License MIT

Motivated by #1516 (comment)
I don't know if it's enough.
Maybe a dedicated section about the behavior of turbo when a LiveAction of a Live Component is returning a RedirectResponse should be more appropriate ?

@carsonbot carsonbot added docs Improvements or additions to documentation LiveComponent Status: Needs Review Needs to be reviewed labels Jan 20, 2025
@zefyx
Copy link
Author

zefyx commented Jan 20, 2025

I'm not sure to understand the RST Linter failing.
Executing locally the command docker run --rm -it -e DOCS_DIR='/docs' -v ${PWD}:/docs oskarstark/doctor-rst -vvv as mentioned in the CONTRIBUTING.md#working-on-documentation didn't trigger any error.

@Kocal
Copy link
Member

Kocal commented Jan 20, 2025

Hi, here some explanations:

  1. for the error Please ensure title "Downloading files" and underline length are matching, it means that the title Downloading files and the underline (usually --------(...), or ~~~~~~~(...)) does not have the same length
  2. for the error Please add 4 spaces for every indention., it looks like there is a space missing for each lines you added in the code blocks.

For the docker run ... from the documentation, I believe you must adjust the DOCS_DIR env variable to point to src/LiveComponent/doc.

@zefyx
Copy link
Author

zefyx commented Jan 21, 2025

Hi @Kocal and thanks.

I don't want to pollute this PR with a non related topic but before fixing the index.rst file for the errors triggered by the github RST Linter check, here is what the local command was outputting before and after the fix.

docker run --rm -it -e DOCS_DIR='/symfony-ux' -v ${PWD}:/symfony-ux oskarstark/doctor-rst -vvv

> Checking Box requirements:
  ✔ This application requires a PHP version matching "^8.3".
  ✔ This application requires the extension "zlib".
  ✔ This application requires the extension "ctype".
  ✔ The package "webmozart/assert" requires the extension "ctype".
  ✔ This application requires the extension "iconv".
  ✔ The package "symfony/dependency-injection" conflicts with the extension "psr".
  ✔ The package "symfony/service-contracts" conflicts with the extension "psr".

                                                                                                                                                                                                            
 [OK] Your system is ready to run the application.                                                                                                                                                          
                                                                                                                                                                                                            

 Analyze *.rst(.inc) files in: /symfony-ux
 Used config file:             /symfony-ux/.doctor-rst.yaml

Analyze /symfony-ux/src/Autocomplete/doc/index.rst
Analyze /symfony-ux/src/Chartjs/doc/index.rst
Analyze /symfony-ux/src/Cropperjs/doc/index.rst
Analyze /symfony-ux/src/Dropzone/doc/index.rst
Analyze /symfony-ux/src/Icons/doc/index.rst
Analyze /symfony-ux/src/LazyImage/doc/index.rst
Analyze /symfony-ux/src/LiveComponent/doc/index.rst
Analyze /symfony-ux/src/Map/doc/index.rst
Analyze /symfony-ux/src/Notify/doc/index.rst
Analyze /symfony-ux/src/React/doc/index.rst
Analyze /symfony-ux/src/StimulusBundle/doc/index.rst
Analyze /symfony-ux/src/Svelte/doc/index.rst
Analyze /symfony-ux/src/Swup/doc/index.rst
Analyze /symfony-ux/src/TogglePassword/doc/index.rst
Analyze /symfony-ux/src/Translator/doc/index.rst
Analyze /symfony-ux/src/Turbo/doc/index.rst
Analyze /symfony-ux/src/TwigComponent/doc/index.rst
Analyze /symfony-ux/src/Typed/doc/index.rst
Analyze /symfony-ux/src/Vue/doc/index.rst
src/Autocomplete/doc/index.rst ✔

src/Chartjs/doc/index.rst ✔

src/Cropperjs/doc/index.rst ✔

src/Dropzone/doc/index.rst ✔

src/Icons/doc/index.rst ✔

src/LazyImage/doc/index.rst ✔

src/LiveComponent/doc/index.rst ✔

src/Map/doc/index.rst ✔

src/Notify/doc/index.rst ✔

src/React/doc/index.rst ✔

src/StimulusBundle/doc/index.rst ✔

src/Svelte/doc/index.rst ✔

src/Swup/doc/index.rst ✔

src/TogglePassword/doc/index.rst ✔

src/Translator/doc/index.rst ✔

src/Turbo/doc/index.rst ✔

src/TwigComponent/doc/index.rst ✔

src/Typed/doc/index.rst ✔

src/Vue/doc/index.rst ✔

                                                                                                                        
 [OK] All files are valid!         

For testing purpose in order to see if the .doctor-rst.yaml was correctly loaded and the *.rst file where checked, when uncommenting some currently commented rules in the .doctor-rst.yaml the checks failed "as expected".
So, i still don't understand why the rules underline length are matching and 4 spaces for every indention where not triggered locally.

Copy link
Member

@Kocal Kocal left a comment

Choose a reason for hiding this comment

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

Minor comments on the code examples, and it should be good.


About the disparities between the local order and the CI, I don't really know, maybe the container version is different? Let's continue talking about this in a dedicated issue. Thanks 🙏🏻

src/LiveComponent/doc/index.rst Outdated Show resolved Hide resolved
src/LiveComponent/doc/index.rst Outdated Show resolved Hide resolved
@Kocal
Copy link
Member

Kocal commented Jan 21, 2025

Status: Needs Work

@carsonbot carsonbot added Status: Needs Work Additional work is needed and removed Status: Needs Review Needs to be reviewed labels Jan 21, 2025
@carsonbot carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Jan 22, 2025
@zefyx
Copy link
Author

zefyx commented Jan 23, 2025

Sorry for missing the unrelated modification introduced with the commit 99d3d21
the reStructuredText formatter of the IDE automatically removed the 2 white spaces when saving.
(consistent but not related to the current PR)

Copy link
Member

@smnandre smnandre left a comment

Choose a reason for hiding this comment

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

The small note to remove and let's merge this! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation LiveComponent Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants