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

feat: headless functionality in hook #45

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jshthornton
Copy link
Contributor

@jshthornton jshthornton commented Aug 26, 2021

This pull request moves the functionality of the oauth logic into a hook. That way this package can be utilised headlessly allowing developers to manage the UI aspect.

I've put this PR into draft until the following have been done:

  • Maintainers are happy with the concept of moving to headless (whilst still providing a component for ease of use)
  • Whether this is classified as a breaking change as it utilises hooks which were introduced into React v16.8.0
  • Tests have been written exclusively for the hook
  • Docs have been updated to include the hook usage

@bhubr
Copy link
Owner

bhubr commented Aug 26, 2021

Hello!

Well, thanks for the work you put in!

  • I initially made this package for my own use. Since then, it seems to have taken a life of its own. Sure, there are not that many users, but I'd need to make sure that we don't break their apps: either by bumping a major version, and/or verifying that they' re using recent React versions (which seems to be the case, by picking a few random repos that use it).
  • I actually hadn't given any thought about going headless. Could you please provide some more background (a use case or an example)? Is it mainly for making tests easier, or is there another aspect to it that I'm totally missing? (sorry if it's a very naive question!)

@bhubr
Copy link
Owner

bhubr commented Aug 26, 2021

By the way, I also have quite a few issues left hanging: mainly #16, #22, #33, #37

So I'm thinking it would be a good moment to address at least some of them, and to include this PR into the overall plan. So that we move forward to 1.0 with both new features & under-the-hood changes.

@bhubr bhubr added enhancement New feature or request question Further information is requested labels Aug 26, 2021
@jshthornton
Copy link
Contributor Author

tbh it isn't even a breaking change, as whilst this PR uses the hooks syntax of use* there isn't actually any hooks utilised under the hood, so versions < v16.8.0 would still work.

The benefits of going headless are that it decouples the functionality from the presentation, allowing for easier testing, as well as for consuming apps to truly implement the rendering component as desired.
In the app I am using this package with I'm having to use the render function instead to get customised DOM to render icons, data-test-id as well as to apply some custom styles on the fly instead of class names.

@jshthornton
Copy link
Contributor Author

I'm happy to help contribute on some of those other features / bugs too when I get a spare moment.

@bhubr
Copy link
Owner

bhubr commented Aug 27, 2021

Thanks for the additional context! I'm pretty busy today but I'll answer more thoroughly as soon as I can (incl. on your latest issue, which I find interesting!)

@bhubr
Copy link
Owner

bhubr commented Nov 11, 2021

Hi @jshthornton

Sorry it took me so long to respond. You probably moved on to other things since then!
I started a new job which kept me awfully busy. The pace is slowing down a bit, so I intend to resume work on the library.

After a quick glance at what you did already, it seems good to me! I'm gonna try and write tests for it.

@DiefBell
Copy link

DiefBell commented Jun 1, 2022

Has any more work been done on this PR? A hook would be great because I'd then be able to quite easily hook it up to https://www.npmjs.com/package/react-social-login-buttons

@bhubr
Copy link
Owner

bhubr commented Jun 1, 2022

@DiefBell I've been (and still am) super-super-busy and haven't done more than manually testing. I expect to finish a fairly big project in about a month. If you have skills in writing tests, you're more than welcome to contribute!

Other options:

Sorry, I really can't devote even the tiniest amount of time to this project right now!

@bhubr
Copy link
Owner

bhubr commented Jan 16, 2023

Hi @jshthornton

Sorry for coming back way after due time...
I'm only finding time and motivation now to resume working on the project.
Since it's one of the biggest changes and I have other plans ahead, I think it's better that I proceed with this issue first.

I pulled your branch and rebased it on the current master branch.
I pushed the rebased branch here: https://github.com/bhubr/react-simple-oauth2-login/tree/feature/52-pr45-jshthornton-headless-functionality-in-hook

Are you OK with me continuing from there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants