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

[Doc] Fix Custom schema example #121

Merged
merged 1 commit into from
Feb 16, 2024
Merged

Conversation

slax57
Copy link
Contributor

@slax57 slax57 commented Jan 24, 2024

I believe the code example in the 'Custom schema' section of the docs is wrong, as the schema prop expects a function (() => string) and not a string directly.

@scheiblr
Copy link
Collaborator

scheiblr commented Jan 24, 2024

Thank you for your concerns.

The documentation here is flawless. A look at the code ( here and here) reveals that it is right. Also, the test cases show it (e.g. here).

The reason to use a function rather than a string is, that like that it is possible to include some function which retrieves the schema name from another storage (such as the localStorage) and allows this value to change.

I hope I was able to clarify the decision and point out, that the documentation is correct.

Kind regards,

@scheiblr
Copy link
Collaborator

closed.

@scheiblr scheiblr closed this Jan 24, 2024
@slax57
Copy link
Contributor Author

slax57 commented Jan 24, 2024

Thank you for your answer.

However, I'm sorry to insist, but I think you might have misunderstood my point.
I'm not questioning the fact that it needs to be a function, on the contrary, I agree!

What I'm saying is that, if you try to apply the code example from the docs directly (i.e. pass schema: localStorage.getItem("schema") || "api"), you will see a TS error telling you that schema cannot be a string.

That's why I suggested to fix that part of the Readme.md, but not the part saying it needs to be a function (with which I fully agree!).

I hope these explanations will make my request clearer.

In any case, it's only a small error, and it's easy to figure out how to fix it in user-land, but still, thought I would open a PR to improve the docs.

@scheiblr
Copy link
Collaborator

Sorry, my mistake. Thanks for insisting :) I'm gonna accept the fix :) thank you very much!

@scheiblr scheiblr reopened this Feb 16, 2024
@scheiblr scheiblr self-assigned this Feb 16, 2024
@scheiblr scheiblr added the documentation Improvements or additions to documentation label Feb 16, 2024
@scheiblr scheiblr merged commit 2b830a0 into raphiniert-com:master Feb 16, 2024
3 checks passed
@coveralls
Copy link

Pull Request Test Coverage Report for Build 7924150770

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 89.216%

Totals Coverage Status
Change from base Build 7359427530: 0.0%
Covered Lines: 265
Relevant Lines: 276

💛 - Coveralls

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

Successfully merging this pull request may close these issues.

3 participants