Skip to content

Fix ordered lists in secrets management and Form component documentation #16944

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

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

alexandre-daubois
Copy link
Member

No description provided.

@alexandre-daubois alexandre-daubois requested a review from xabbuh as a code owner July 7, 2022 07:02
@carsonbot carsonbot added this to the 4.4 milestone Jul 7, 2022
@javiereguiluz
Copy link
Member

Thanks Alexandre for this proposal. However, this formatting was used explicitly. The reason is that some of the list items are very long, so it requires to add some indenting to all contents. The current syntax makes things easier to maintain.

Also, we switched recently to a custom PHP-based RST parser, so using the indenting format could create issues (supporting RST elements such as admonitions, sublists, images, etc. as subelements of a list item is not trivial at all. we might be missing some parsing logic and it's very difficult to fix it).

That's why we're closing this without merging, but we hope you understand the reasons. Thanks!

@alexandre-daubois
Copy link
Member Author

Thank you for your comment @javiereguiluz! Totally understand the reasons. Fortunately, I could only find the display problem of multiple "1." at these spots in the documentation, which is very limited. Good luck for the fix in the parser!

@javiereguiluz javiereguiluz reopened this Jul 8, 2022
@javiereguiluz javiereguiluz merged commit a0d07bf into symfony:4.4 Jul 8, 2022
@javiereguiluz
Copy link
Member

Alexandre, I was wrong when looking at your pull request, so I closed it too early. My previous comment is true in general, but in this case you did the right thing to fix a bug and the indented contents of the list items are simple enough for our parser, so there shouldn't be any issue.

Sorry ... and thank you! It's merged now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants