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 idps option for config for Signin widget flow #572

Closed
wants to merge 1 commit into from

Conversation

denysoblohin-okta
Copy link
Contributor

@denysoblohin-okta denysoblohin-okta commented Dec 23, 2020

Added idps to for sample config UI for Signin Widget flow.


Screenshot 2020-12-23 at 04 42 57

Screenshot 2020-12-23 at 04 44 41

Copy link
Contributor

@oleksandrpravosudko-okta oleksandrpravosudko-okta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
would you mind adding a selenium test verifying idp window opens on idp button click?

@codecov-io
Copy link

codecov-io commented Jan 15, 2021

Codecov Report

Merging #572 (a4abac3) into master (4ee1687) will increase coverage by 0.73%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #572      +/-   ##
==========================================
+ Coverage   92.96%   93.70%   +0.73%     
==========================================
  Files          38       42       +4     
  Lines        2161     3017     +856     
  Branches      452      711     +259     
==========================================
+ Hits         2009     2827     +818     
- Misses        152      190      +38     
Impacted Files Coverage Δ
lib/server/serverStorage.ts 86.95% <0.00%> (-13.05%) ⬇️
lib/browser/browserStorage.ts 96.96% <0.00%> (-3.04%) ⬇️
lib/oauthUtil.ts 97.98% <0.00%> (-0.49%) ⬇️
lib/pkce.ts 100.00% <0.00%> (ø)
lib/constants.ts 100.00% <0.00%> (ø)
lib/types/index.ts 100.00% <0.00%> (ø)
lib/server/server.ts 100.00% <0.00%> (ø)
lib/fetch/fetchRequest.ts 100.00% <0.00%> (ø)
lib/TransactionManager.ts 91.17% <0.00%> (ø)
lib/StorageManager.ts 100.00% <0.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ee1687...a4abac3. Read the comment docs.

Comment on lines 216 to 224
idps: config.idps.split(/\s+/).map(v => {
const parts = v.split(/:/);
if (parts.length == 2) {
return {type: parts[0], id: parts[1]};
}
return null;
}).filter(v => v)
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: (very nit). I like to keep the visual focus on the relevant parts of code to make it easier for the next coder who is skimming over the code and may not be familiar with it. What do you think of this:

config.idps.split(/\s+/).map(idpToken => { 
    const [type, id] = idpToken.split(/:/);
    if(!type || !id) { 
       return null;
    }
    return { type, id};
}).filter(v => v);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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.

6 participants