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

[Console] Document console cursor #15270

Merged
merged 1 commit into from Jun 17, 2021
Merged

Conversation

noniagriconomie
Copy link
Contributor

fixes #13529
feature symfony/symfony#27444

ping @wouterj I've seen your slack recently and wanted to give it a try as I will use the feature very soon :)
feel free to comment, specialy if I need to document all the cursor features or not

text and image taken from the feature PR description
inspired by https://symfony.com/doc/current/components/console/helpers/progressbar.html

@Nyholm
Copy link
Member

Nyholm commented Apr 21, 2021

(The “code block” ci job does not support adding new files. I’ll fix that in a separate pr)

@Nyholm
Copy link
Member

Nyholm commented Apr 21, 2021

The CI job is updated.

Regarding your PR: Do we need to create a new page or is there an existing page we can add this content to?

@noniagriconomie
Copy link
Contributor Author

@Nyholm Thank you for the CI
for your second point, I personally checked the https://symfony.com/doc/current/components/console.html#learn-more list and found it will be great to add it here

If you or other found a better place,, I will be pleased to swap the code of course :)

@Nyholm
Copy link
Member

Nyholm commented Apr 22, 2021

Oh. Hmm.. I see.

There is a greater plan to make everything under one document. I'll wait for input from the docs team.


Btw, test retrigger the CI with:

git commit --allow-empty  -m "Trigger CI"
git push

@noniagriconomie noniagriconomie changed the title Document console cursor [Console] Document console cursor Apr 22, 2021
@Nyholm
Copy link
Member

Nyholm commented Apr 22, 2021

Cool. Another CI error. We dont support non-rst files =)

I should filter out these too.

@Nyholm
Copy link
Member

Nyholm commented Apr 22, 2021

May I bother you to trigger the CI again?

@noniagriconomie
Copy link
Contributor Author

@Nyholm here you are :)
feel free to edit/commit on this PR if you can

@Nyholm
Copy link
Member

Nyholm commented Apr 22, 2021

Wohoo. The CI is green. Thank you for helping me.


I am also happy with the content of this PR.
👍🏽

Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Thanks a lot for starting this @noniagriconomie!

I think we should document all available methods. I've made some suggestions on how to do this somewhat briefly below (to avoid long boring listings).

I'm fine with keeping it in a separate document. It seems the most logical from the current Console documentation structure. We can always revisit this when we start working on the Console docs

components/console/helpers/cursor.rst Outdated Show resolved Hide resolved
components/console/helpers/cursor.rst Show resolved Hide resolved
components/console/helpers/cursor.rst Outdated Show resolved Hide resolved
@carsonbot carsonbot changed the title [Console] Document console cursor Document console cursor Apr 24, 2021
@wouterj wouterj added this to the 5.2 milestone Apr 24, 2021
@carsonbot carsonbot changed the title Document console cursor [Console] Document console cursor Apr 24, 2021
@noniagriconomie
Copy link
Contributor Author

noniagriconomie commented Apr 27, 2021

@wouterj many thanks for the reviews, I've tried to address them all
status: review

edit: I do not understand the CI sphinx error

Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Thanks! 2 minor comments left, ready to merge imho :)

components/console/helpers/cursor.rst Show resolved Hide resolved
Comment on lines +72 to +75
// move the cursor to a specific position from its current position
$cursor->moveToPosition(7, 11);
Copy link
Member

Choose a reason for hiding this comment

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

I haven't used this feature, but I think we have to clearify this a bit. This means 7 lines down, and 11 columns to the right? And I can use negative numbers to achieve the opposite?

Or does this expect a coordinate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wouterj it means 7 cols right, 11 rows down from console init cursor
the cursor always moves position from this 0:0 position, thus need an x:y (here positive values) from the init yes

@noniagriconomie
Copy link
Contributor Author

friendly ping @OskarStark I may hope you are the best to ping here related to the error I have:

Warning: ] Found "1" invalid file!                                              
Error: Please do not use ".. code-block:: php", use "::" instead.

I've check other code bloc and for me it is "correct" here, or what am I doing wrong? many thanks :)

@wouterj wouterj changed the base branch from 5.4 to 5.2 June 17, 2021 11:02
@wouterj wouterj requested a review from xabbuh as a code owner June 17, 2021 11:02
wouterj added a commit that referenced this pull request Jun 17, 2021
@wouterj wouterj merged commit fb749b5 into symfony:5.2 Jun 17, 2021
@wouterj
Copy link
Member

wouterj commented Jun 17, 2021

Thank you @noniagriconomie ! I'm sorry this took a while...

The build error appears to be a bug in DOCtor (@OskarStark), I've silenced the rule for now (along with some minor changes) in 7565926

@noniagriconomie noniagriconomie deleted the console-cursor branch June 17, 2021 11:55
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Jun 17, 2021
* upstream/5.2:
  [symfony#15270] Minor fixes
  [Console] Document console cursor
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Jun 17, 2021
* 5.2:
  Minor tweaks
  Adds a heading for stampede prevention so I'm able to reference this section easily
  [symfony#15270] Minor fixes
  [Console] Document console cursor
  Update routing.rst
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Jun 17, 2021
* 5.3:
  Minor tweaks
  Adds a heading for stampede prevention so I'm able to reference this section easily
  [symfony#15270] Minor fixes
  [Console] Document console cursor
  [Security] changed interface name to the correct one
  [Form] Fixed a code example related to CSRF
  Update routing.rst
  Typo
javiereguiluz added a commit to javiereguiluz/symfony-docs that referenced this pull request Jun 17, 2021
* 5.4:
  Minor tweaks
  Adds a heading for stampede prevention so I'm able to reference this section easily
  [symfony#15270] Minor fixes
  [Console] Document console cursor
  [Security] changed interface name to the correct one
  [Form] Fixed a code example related to CSRF
  Update routing.rst
  Typo
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.

[Console] Add Cursor class to control the cursor in the terminal
4 participants