App selections should update current auth states #220

domharrington · 2019-05-31T01:29:40Z

Changing a selection in the Logs dropdown should update current auth states and code sample.
A dropdown of the user's apps/API keys should be put into the auth box and experience the same state changes as the Logs dropdown.
Password inputs on Basic HTTP auth are now input[type=password] inputs.

🚧 Revised design

This breaks lots of tests in index.test.jsx because now you cannot update auth We need to come up with a way to apply changes to this object after getAuth() is called. Maybe we need to merge changes made to the form with the oas files

mjcuva · 2019-11-08T23:10:52Z

Two issues with this for the next time @domharrington and I talk about this (forgetting we've done this twice now):

API Key isn't editable in the auth dropdown
- We should make it editable if logged out/no api keys in jwt
- We should make it a picker for the api key if logged in with jwt
Logs dropdown doesn't update api key (or variables)

erunion · 2019-11-22T00:32:52Z

packages/api-explorer/src/Doc.jsx

@@ -353,7 +356,6 @@ Doc.propTypes = {
  }),
  auth: PropTypes.shape({}).isRequired,
  baseUrl: PropTypes.string,
-  changeGroup: PropTypes.func.isRequired,


Renamed this throughout so it's clearer that this is intended for an onChange event.

erunion · 2019-11-22T00:33:09Z

packages/api-logs/index.jsx

@@ -88,7 +88,7 @@ class Logs extends React.Component {
  }

  onSelect(event) {
-    this.changeGroup(event.target.value);
+    this.onGroupChange(event.target.value, event.target.options[event.target.selectedIndex].text);


Since getAuth asks for a selectedApp param, it makes more sense to push back the group name as well instead of only the group ID/api key to the explorer.

erunion · 2019-11-22T00:36:46Z

@domharrington @mjcuva I'm working on adding some more unit tests, but this is ready for review.

@rafegoldberg We might want to change the font color of the dropdown in the AuthBox to match the color of it in the Logs module.

erunion · 2019-11-22T18:00:30Z

With multiple securities on an operation, it kind of looks like garbage. @madisonyocum @mjcuva @rafegoldberg ideas?

Also changing those dropdowns right now in that example updates both securities. That seems wrong.

domharrington

The code for this looks sound, but I've not tested.

What does this do when there's no logged in user?

Not sure what to do about the multiple schemes thing either. I think in your example it's worse because they're named the same thing, would it be any better if they were different?

Is there value in showing both the and the dropdown? Maybe just showing the dropdown is enough, since they'd see the API key in the code sample?

Is the disabled right now when it's coming from one of these dropdown values?

packages/api-explorer/src/AuthBox.jsx

domharrington · 2019-11-22T20:08:00Z

packages/api-explorer/src/lib/get-auth.js

+              {
+                ...oas.components.securitySchemes[scheme],
+                _key: scheme,
+                selectedApp,


Is this needed?

Yeah, getSingle needs to know the current app so it can pick out the right key.

domharrington · 2019-11-22T20:10:12Z

packages/api-logs/index.jsx

@@ -88,7 +88,7 @@ class Logs extends React.Component {
  }

  onSelect(event) {
-    this.changeGroup(event.target.value);
+    this.onGroupChange(event.target.value, event.target.options[event.target.selectedIndex].text);


Can the group name be inferred from the value higher up the stack? Or does it make sense to always have to pass in the pair here?

packages/api-logs/package.json

erunion · 2019-11-22T23:10:25Z

Talked offline with @mjcuva about the dupe dropdown issue and we're going to play with moving the dropdown into the top right of the authbox and only show it once.

Co-Authored-By: domharrington <domharrington@users.noreply.github.com>

erunion · 2019-11-25T21:45:12Z

@domharrington @rafegoldberg @mjcuva @madisonyocum This is good for review again. The design has slightly changed per our conversation last week to move the groups dropdown up into the first auth header:

Functionality remains the same though, and changing a group in there will swap out the auth state in the app and update both inputs.

rafegoldberg · 2019-11-25T22:50:42Z

@madisonyocum how about something like this?

erunion · 2019-11-25T23:19:46Z

I really, really, really like that. The toggle on the auth scheme entry should probably only show up if there's more than one though.

erunion · 2019-11-26T18:01:25Z

@rafegoldberg @madisonyocum That dropdown isn't going to be present for logged out users, so we should take that into consideration.

- restructure markup and add classes - move in to AuthBox/ directory; add stylesheet

rafegoldberg · 2019-12-06T18:44:56Z

Moved the <AuthBox> component JSX in to AuthBox/index.jsx and added an AuthBox.scss for styling per my prior mockup!

rafegoldberg

Tested locally; e'erything runs well and looks good on my end. Only question I have is whether the group selector will/should be visible in the auth box if there's only a single project? (Couldn't quite figure out how to test this locally...) But it's nothing to hold your merge over!

erunion · 2019-12-06T19:16:57Z

@rafegoldberg We definitely shouldn't show the dropdown if they just have a single project. That looks to be the current behavior in GroupsList, just the demo site always uses two projects to test with. We should make sure the lack of a dropdown in the authbox still looks ok.

This all looks great, though on endpoints with only a single auth scheme, there's a scrollbar that shows up:

only show the dropdown when `props.groups.length` is greater than 1

rafegoldberg · 2019-12-06T19:52:11Z

@erunion fixed the erroneous scrollbars and moved the <GroupList> in to a conditional on groups.length; this should be good to go now!

erunion · 2019-12-06T19:54:12Z

Looks good!

Wired up getAuth() to pass in selected app

deeb4de

This breaks lots of tests in index.test.jsx because now you cannot update auth We need to come up with a way to apply changes to this object after getAuth() is called. Maybe we need to merge changes made to the form with the oas files

Merge branch 'master' into feature/wire-up-selected-app-to-auth

cb7d484

erunion self-assigned this Nov 20, 2019

erunion added the type:enhancement A potential new feature to be added, or an improvement we could make label Nov 20, 2019

feat: changing the logs dropdown now updates the authbox and code sample

2699bc2

erunion changed the title ~~Wired up getAuth() to pass in selected app~~ App selections should update current auth states Nov 21, 2019

erunion added the type:bug Something isn't working label Nov 21, 2019

feat: adding an app dropdown into the authbox

b51453c

erunion reviewed Nov 22, 2019

View reviewed changes

erunion marked this pull request as ready for review November 22, 2019 00:35

erunion requested review from rafegoldberg and mjcuva November 22, 2019 00:35

Merge branch 'master' into feature/wire-up-selected-app-to-auth

82f8ffd

domharrington commented Nov 22, 2019

View reviewed changes

erunion and others added 6 commits November 24, 2019 17:49

fix: Update packages/api-explorer/src/AuthBox.jsx

2bb3ebe

Co-Authored-By: domharrington <domharrington@users.noreply.github.com>

Merge branch 'master' into feature/wire-up-selected-app-to-auth

6a4b887

fix: fixing some typod object usage

92fa995

fix: moving the group dropdown up to the header and showing it once

88da9c6

fix: reverting changes to the api-logs module since its not needed

cab3b2a

fix: simplifying group changes

c1622f2

fix: reverting jest to run in band

b145f3e

fix: excluding dotfiles from the swagger-files directory

4f584d2

erunion and others added 2 commits November 27, 2019 09:34

feat: basic http auth password inputs are now masked

eb6e3a9

style(AuthBox)

9bf52ff

- restructure markup and add classes - move in to AuthBox/ directory; add stylesheet

refactor(AuthBox): fix prop ordering per eslint

1bb2390

rafegoldberg approved these changes Dec 6, 2019

View reviewed changes

rafegoldberg added 4 commits December 6, 2019 11:38

style(AuthBox): remove scroll bar + max-height

e2b5db3

fix(AuthBox): conditionally display GroupList select

082b583

only show the dropdown when `props.groups.length` is greater than 1

style(AuthBox): lighten background color

d3a6cb7

test(AuthBox): make sure groups exists

c55f4e8

erunion merged commit c97ff83 into master Dec 6, 2019

erunion deleted the feature/wire-up-selected-app-to-auth branch December 6, 2019 19:54

erunion mentioned this pull request Jul 14, 2020

chore(deps): upgrading @readme/oas-tooling to 3.5.2 #834

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

App selections should update current auth states #220

App selections should update current auth states #220

domharrington commented May 31, 2019 •

edited by erunion

Loading

mjcuva commented Nov 8, 2019

erunion Nov 22, 2019

erunion Nov 22, 2019

erunion commented Nov 22, 2019

erunion commented Nov 22, 2019

domharrington left a comment

domharrington Nov 22, 2019

erunion Nov 25, 2019

domharrington Nov 22, 2019

erunion commented Nov 22, 2019

erunion commented Nov 25, 2019

rafegoldberg commented Nov 25, 2019 •

edited

Loading

erunion commented Nov 25, 2019

erunion commented Nov 26, 2019

rafegoldberg commented Dec 6, 2019 •

edited

Loading

rafegoldberg left a comment •

edited

Loading

erunion commented Dec 6, 2019

rafegoldberg commented Dec 6, 2019

erunion commented Dec 6, 2019

App selections should update current auth states #220

App selections should update current auth states #220

Conversation

domharrington commented May 31, 2019 • edited by erunion Loading

🚧 Revised design

mjcuva commented Nov 8, 2019

erunion Nov 22, 2019

Choose a reason for hiding this comment

erunion Nov 22, 2019

Choose a reason for hiding this comment

erunion commented Nov 22, 2019

erunion commented Nov 22, 2019

domharrington left a comment

Choose a reason for hiding this comment

domharrington Nov 22, 2019

Choose a reason for hiding this comment

erunion Nov 25, 2019

Choose a reason for hiding this comment

domharrington Nov 22, 2019

Choose a reason for hiding this comment

erunion commented Nov 22, 2019

erunion commented Nov 25, 2019

rafegoldberg commented Nov 25, 2019 • edited Loading

erunion commented Nov 25, 2019

erunion commented Nov 26, 2019

rafegoldberg commented Dec 6, 2019 • edited Loading

rafegoldberg left a comment • edited Loading

Choose a reason for hiding this comment

erunion commented Dec 6, 2019

rafegoldberg commented Dec 6, 2019

erunion commented Dec 6, 2019

domharrington commented May 31, 2019 •

edited by erunion

Loading

rafegoldberg commented Nov 25, 2019 •

edited

Loading

rafegoldberg commented Dec 6, 2019 •

edited

Loading

rafegoldberg left a comment •

edited

Loading