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

Refactor params #792

Closed
wants to merge 30 commits into from
Closed

Refactor params #792

wants to merge 30 commits into from

Conversation

jessyao26
Copy link

Implementation
I added a constants files separated into categories of different constants (i.e. oauth2, auth0, blacklist).
In another file, I created a params object that contains param arrays with separated by property for each type of param array.
Note
The params object makes more sense when it's not too nested like paramsArray.blacklist.popup.handler.base. It gets really nested with certain properties because their param arrays only differ by one or two values. I'm worried this is confusing, but am unsure of a different way to implement it right now. Let me know what you think, feel free to give feedback

@luisrudge
Copy link
Contributor

I agree this is confusing (when you use a lot of them), we should probably just use the same values all around, but it's hard to take a decision on which params to use right now. I'll evaluate this approach during this week (I'm super busy this week, sorry in advance).

@jessyao26
Copy link
Author

jessyao26 commented Jul 9, 2018 via email

@luisrudge luisrudge requested a review from a team May 20, 2019 16:14
@luisrudge
Copy link
Contributor

I'm sorry it took that much time to review this. After talking with some colleagues, we decided that this approach would be very confusing with the current state of the SDK. The real issue is that we have a lot of different overrides all around and that is the main source of confusion. Thanks so much for your effort and I'm sorry we're not going through with this amazing PR. 🎉

@luisrudge luisrudge closed this May 20, 2019
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.

2 participants