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

Update Changelog, Docs and Error Message #153

Merged

Conversation

pulkitkkr
Copy link
Contributor

This PR Addresses Docs, Error Messages and Changelogs-related Issues reported by @justin808 in #152 and #152

@pulkitkkr pulkitkkr force-pushed the pulkitkkr_misc_updates_for_append_stylesheet_pack_tag branch from 1073953 to b6f3114 Compare June 30, 2022 21:01
@pulkitkkr pulkitkkr requested a review from justin808 June 30, 2022 21:03
README.md Outdated
@@ -284,8 +284,7 @@ Thus, you can distribute the logic of what packs are needed for any route. All t

**Important:** `append_javascript_pack_tag` can be used anywhere in your application as long as it is executed BEFORE the `javascript_pack_tag`. If you attempt to call `append_javascript_pack_tag` helper after `javascript_pack_tag`, an error will be raised. You should have only a single `javascript_pack_tag` invocation in your page load.

**Important** Similar to `append_javascript_pack_tag`, the `append_stylesheet_pack_tag` should be used anywhere in your application as long as it is executed BEFORE the `stylesheet_pack_tag`. However, if you attempt to call `append_stylesheet_pack_tag` helper after `stylesheet_pack_tag`, an error will be **NOT** be raised.

**Important** Similar to `append_javascript_pack_tag`, the `append_stylesheet_pack_tag` should be used anywhere in your application as long as it is executed BEFORE the `stylesheet_pack_tag`. If you attempt to call `append_stylesheet_pack_tag` helper after `stylesheet_pack_tag`, an error will be raised. However, you may have multiple invocation of `stylesheet_pack_tag`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now you have basically duplicate paragraphs one after another. Maybe combine them?

Copy link
Member

Choose a reason for hiding this comment

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

100% agree with @alexeyr-ci1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True! We should merge them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be honest, I wanted to recommend it already on the previous PR, but decided not to because of the apparent difference.

@@ -133,7 +133,7 @@ def test_append_javascript_pack_tag_raises

assert_equal \
"You can only call append_javascript_pack_tag before javascript_pack_tag helper. " +
"Please refer to https://github.com/shakacode/shakapacker/blob/master/README.md#usage for the usage guide",
"Please refer to the documentation for append_javascript_pack_tag for more information.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this documentation, though? Do we have an API documentation page? (applies to all 3)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@alexeyr-ci1 we'll want to look into better organization of documentation in the future. We host the React on Rails documentation on our website: https://www.shakacode.com/react-on-rails/docs/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what do you guys suggest for now? Should we add the link to documentation page on Shakacode site?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, this question is about what to do for now (though I've filed #154 while we were at it).

Copy link
Collaborator

Choose a reason for hiding this comment

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

So what do you guys suggest for now? Should we add the link to documentation page on Shakacode site?

It doesn't have these docs for now, I'd link to the correct README part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will revert back to have the Links and just point to the correct location in README.md

Copy link
Member

Choose a reason for hiding this comment

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

I bet we change the docs later and forget to update the error messages and tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mention it should be changed in #154 (comment).

@justin808 justin808 merged commit 3473ce1 into master Jul 1, 2022
@justin808 justin808 deleted the pulkitkkr_misc_updates_for_append_stylesheet_pack_tag branch July 1, 2022 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants