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 Select::id() #130

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Add Select::id() #130

wants to merge 1 commit into from

Conversation

richardhj
Copy link
Contributor

Shorthand method for when you want to reference a select by its id rather than textual representation.

@what-the-diff
Copy link

what-the-diff bot commented Apr 10, 2023

PR Summary

  • Added a new method to the Select class
    This method, called id, takes in a parameter of type string and returns an instance of the Select class.
  • Improved selectItem object configuration
    The new method creates a selectItem object, sets its ID property with the provided value, and assigns it as content for the current selectProperty object.

@johguentner johguentner added the tests-required Tests for this PR are required label Apr 10, 2023
@johguentner
Copy link
Member

Thanks @richardhj for the PR!

I'd change the name to ::valueById(...) to make it more clear what is done. Just ::id(...) could be confusing, because it breaks apart from the pattern of creating new properties by ::valueSomething(...).

We'd plan to merge this with 1.1.0. Please add tests to your implementation, so we can merge with the dev branch.

@johguentner
Copy link
Member

Hi @richardhj,

just wanted to check with you, if you want to write tests for this feature.
Within our contribution guidlines (https://github.com/5am-code/laravel-notion-api/blob/main/CONTRIBUTING.md) you can double check what is important for a PR to this repo. We want to keep this repo as clean as possible, so we want PRs to stick to those guidelines as best as possible - aside from some exceptions.

If you need any help, please let us know.

We will push v1.1.0 around the end of the week and will cleanup all remaining PRs marked for v1.1.0.

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests-required Tests for this PR are required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants