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

Add missing audio_tag, favicon_link_tag, video_tag methods in ActionView helper guide [ci skip] #49925

Conversation

akhilgkrishnan
Copy link
Member

Detail

This Pull Request adds audio_tag, favicon_link_tag and video_tag in ActionView helper method guide.

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the docs label Nov 5, 2023
@akhilgkrishnan akhilgkrishnan force-pushed the add-missing-methods-in-view-helper-guide branch 2 times, most recently from 320de06 to c7c80ae Compare November 8, 2023 02:31
@akhilgkrishnan akhilgkrishnan force-pushed the add-missing-methods-in-view-helper-guide branch from c7c80ae to c7ff543 Compare November 10, 2023 09:27
@akhilgkrishnan akhilgkrishnan force-pushed the add-missing-methods-in-view-helper-guide branch from c7ff543 to 8641cf8 Compare November 12, 2023 18:54
@akhilgkrishnan akhilgkrishnan force-pushed the add-missing-methods-in-view-helper-guide branch from 8641cf8 to cf605cd Compare November 18, 2023 16:40
@akhilgkrishnan
Copy link
Member Author

@vipulnsward Updated all the requested changes.

@vipulnsward vipulnsward merged commit 21b72e4 into rails:main Nov 20, 2023
3 checks passed
@zzak
Copy link
Member

zzak commented Nov 20, 2023

Not wanting to be dissuasive here, but I think reproducing documentation that should already exist in the guides is an old habit we should try to avoid. Are these methods already available on api.rubyonrails.org? Can we just link to the appropriate section to "learn more"? (Are types of questions we should be asking ourselves).

Thank you for your work on the docs! 🙇

@vipulnsward
Copy link
Member

Not wanting to be dissuasive here, but I think reproducing documentation that should already exist in the guides is an old habit we should try to avoid. Are these methods already available on api.rubyonrails.org? Can we just link to the appropriate section to "learn more"? (Are types of questions we should be asking ourselves).

Sorry, yeah I agree, and was going to follow up with changes myself. There is missing things here that better linked to "Learn more".

@zzak Please feel free to revert if needed, I can send a new change, thanks for the comment 🙇

@akhilgkrishnan akhilgkrishnan deleted the add-missing-methods-in-view-helper-guide branch November 20, 2023 13:50
@akhilgkrishnan
Copy link
Member Author

Not wanting to be dissuasive here, but I think reproducing documentation that should already exist in the guides is an old habit we should try to avoid. Are these methods already available on api.rubyonrails.org? Can we just link to the appropriate section to "learn more"? (Are types of questions we should be asking ourselves).

Thank you for your work on the docs! 🙇

Yes, I agree with you. This is only just an abstract documentation which references to API documentation. WDYT about removing this documentation completely?
cc: @zzak @vipulnsward

@zzak
Copy link
Member

zzak commented Nov 25, 2023

I don't think it's worth reverting. Unless we're changing the visibility of something (public/private), then all of this is kind of fluid and we can continue to iterate and improve the documentation as we go. ❤️

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

Successfully merging this pull request may close these issues.

None yet

4 participants