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 README #363

Merged
merged 5 commits into from
Jan 17, 2024
Merged

Update README #363

merged 5 commits into from
Jan 17, 2024

Conversation

jon-sully
Copy link
Contributor

Pull Request

Summary:
Still a bit WIP but wanted to pretty significantly revamp the main repo README with the V2 changes

There's still a lot more content to work on, but I at least wanted to get this up!

Related Issue:
n/a

Description:
V2 changed a lot of stuff! Felt like the whole README needed a big look and revamp ❤️

Testing:
n/a

Screenshots (if applicable):
Recommend reading from scratch instead of reading via diffs: https://github.com/jon-sully/noticed/tree/update-readme

@excid3
Copy link
Owner

excid3 commented Jan 16, 2024

Looking good so far!

README.md Outdated Show resolved Hide resolved
@jon-sully
Copy link
Contributor Author

One thing I'll ask for clarity on, @excid3. I don't recall if it was required or optional, but we used explicit params in my app's V1 Notifications. In V2 explicit params are totally optional and declared instead under the macro required_params (or singular required_param) instead of just params, right?

class NewCommentNotifier < Noticed::Event
  deliver_by :email do |config|
    config.mailer = "CommentMailer"
		config.if = ->(recipient) { !!recipient.preferences[:email] }
  end

  required_params :record, :foo
end

@jon-sully
Copy link
Contributor Author

Planning on wrapping this up today, btw!

@excid3
Copy link
Owner

excid3 commented Jan 16, 2024

Sounds good. 👍

params from v1 was just renamed to required_params for clarity. They both trigger validation to check and ensure the param name was in the params hash.

@jon-sully jon-sully marked this pull request as ready for review January 16, 2024 22:26
@jon-sully
Copy link
Contributor Author

I think I'm about done here! Massive changes to the README, a few tweaks to the actual library code as prompted by the README.. overall a good turnaround!

@excid3
Copy link
Owner

excid3 commented Jan 16, 2024

I'll give it a review after the kiddo goes to bed. 👍

README.md Outdated Show resolved Hide resolved
@jon-sully
Copy link
Contributor Author

Snuck in one more commit to add some details to the V2 upgrade guide with the more recent changes 👍

@excid3 excid3 merged commit b8ca2d6 into excid3:main Jan 17, 2024
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