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 some typos in helpers/html_helper.rst #8916

Closed
wants to merge 2 commits into from

Conversation

obozdag
Copy link
Contributor

@obozdag obozdag commented May 26, 2024

Description
Fix some typos in helpers/html_helper.rst

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Fix some typos in helpers/html_helper.rst
Add alt attribute to img element in helpers/html_helper/002.php
@kenjis kenjis added the documentation Pull requests for documentation only label May 27, 2024
Comment on lines -278 to +288
.. php:function:: embed($src = ''[, $type = false[, $attributes = ''[, $indexPage = false]]])
.. php:function:: track($src, $kind, $srcLanguage, $label)

:param string $src: The path of the media resource
:param string $kind: The kind of timed track
:param string $srcLanguage: The language of the timed track
:param string $label: A user-readable title for the timed track
:returns: An HTML track element
:rtype: string

Generates a track element to specify timed tracks. The tracks are
formatted in WebVTT format. Example:

.. literalinclude:: html_helper/019.php
Copy link
Member

@kenjis kenjis May 27, 2024

Choose a reason for hiding this comment

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

These differences make it difficult to review.
I don't know why track() location was changed.
Please create one commit for changing the location.

Also, there are too many diffs in one commit.
Many fixes are corrections of errors, and they are too large to be called typos.
Please create a commit with a commit message of why you changed for every single meaningful fix.

Presumably, this would be if you were using a media controller:
There is an optional second parameter, a true/false value, that
specifies if the *src* should have ``$config['indexPage']`` added to the address it creates.
Presumably, this would be true if you were using a media controller???:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Presumably, this would be true if you were using a media controller???:
Presumably, this would be true if you were using a media controller:

@kenjis kenjis added the needs rework Changes requested by reviewer that are still pending label Jun 2, 2024
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

Could you please re-create this PR so that each meaningful change is committed as one git commit? Because I cannot review this PR.

E.g.,
a commit to move a paragraph.
a commit to replace tags with elements.
a commit to fix one function description.

@kenjis kenjis added the waiting for info Issues or pull requests that need further clarification from the author label Aug 12, 2024
@github-actions github-actions bot added the stale Pull requests with conflicts label Aug 27, 2024
Copy link

👋 Hi, @obozdag!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@kenjis
Copy link
Member

kenjis commented Sep 6, 2024

Closed by #9153, #9161, #9162, #9163, #9164

@kenjis kenjis closed this Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests for documentation only needs rework Changes requested by reviewer that are still pending stale Pull requests with conflicts waiting for info Issues or pull requests that need further clarification from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants