Feature/session provider by maidbarion · Pull Request #16 · inrupt-archived/solid-ui-react

maidbarion · 2020-07-23T19:30:39Z

Hey Jack, just raising a draft PR for feedback like last time. Testing to follow, just wanting some feedback while you're still in the office.

ajacksified

Looks good! Just a couple comments - remove src/components/dashboard, and restore the login and logout button stories.

ajacksified · 2020-07-23T19:59:07Z

-import React, { ReactElement } from "react";
-import { Button } from "@material-ui/core";
-import LoginButton from "../src/logIn";
+import React from "react";


These stories should still exist separately so that a user can see the documentation for login/logout buttons.

Additionally, this code - because it's documentation-specific, not a component any user would actually use - should live in the SeessionProvider story.

ajacksified · 2020-07-23T20:00:24Z

+      setSessionRequestInProgress(false);
      onLogin();
    } catch (error) {
      onError(error);


setSessionRequestInProgress(false) should also be called if an error occurs.

andydavison · 2020-07-24T14:24:27Z

 export default {
  LoginButton,
  LogoutButton,
+  SessionContext,


I'm guessing this file is the list of 'public' stuff for the module/sdk? In which case, we want the SessionProvider/useSession in here (dunno about the SessionContext)

cool, removed context and added the provider and hook

andydavison · 2020-07-24T14:27:28Z

+      <BodyType>{sessionRequestText}</BodyType>
+    </div>
+  );
+}


this can stay here for now (file's not too long, and makes it easier to see the full picture in one file), but one for a discussion at some point I think — we might want to set up a common folder in /stories for this sort of demo component, or give each story it's own folder where these can live

Agreed, I think it will be a bit cleaner splittling it into a different folder. Will leave it for now, but a discussion to be had later.

I think that this is a very specific component used to show how useSession is used, so sharing isn't necessarily useful. Additionally, in order for the documentation to be useful, we need the source displayed, and keeping the definition in a separate file makes that more difficult (if Storybook can even handle it.)

Each component should have a story with minimal "noise"; that is to say, add just enough code to show all possible configurations for the component you're documenting, and not anything more. In this case, as per my comments above, you should remove the styling, onLogin, onError, etc - this is meant to show SessionProvider, not how to inject JSS into headings.

maidbarion · 2020-07-24T15:02:56Z

Hey Jack, if you're around - could you take a look at this?

ajacksified · 2020-07-24T16:36:13Z

+}
+
+function Dashboard() {
+  const { session, sessionRequestInProgress } = useSession();


When looking at the generated documentation, this example doesn't actually show how useSession is actually getting used:

I think you should minimize the extra code so that the focus is on the SessionProvider:

Get rid of the onLogin / onError functions

Get rid of the styling

Additionally, we may need to replace @storybook/addon-docs with @storybook/addon-storysource in order to display the actual source code, so that the Dashboard function- which is what developers are likely most interested in- is displayed in the documentation.

Hey Jack, Iatest version has the syling removed and I've replaced addon-docs with addon-storysource. I noticed in the docs, that the actions pane isn't being utilised at the moment - will this be required for other components, or should we remove it?

I think it could be useful for onLogin, onLogout, onSave, and onError - not sure yet. I'd leave it for now. We can remove it later if we don't end up using it.

ajacksified · 2020-07-24T16:39:15Z

+      <BodyType>{sessionRequestText}</BodyType>
+    </div>
+  );
+}


I think that this is a very specific component used to show how useSession is used, so sharing isn't necessarily useful. Additionally, in order for the documentation to be useful, we need the source displayed, and keeping the definition in a separate file makes that more difficult (if Storybook can even handle it.)

Each component should have a story with minimal "noise"; that is to say, add just enough code to show all possible configurations for the component you're documenting, and not anything more. In this case, as per my comments above, you should remove the styling, onLogin, onError, etc - this is meant to show SessionProvider, not how to inject JSS into headings.

ajacksified · 2020-07-27T14:34:24Z

+    <SessionProvider>
+      <LoginButton
+        popupUrl="./popup.html"
+        onLogin={() => console.log("On Login")}


onlogin / onlogout should be removed from the SessionProvider story.

vercel · 2020-07-29T08:50:03Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

ajacksified · 2020-07-30T12:09:38Z

@@ -1,83 +0,0 @@
-/**


Why were these tests deleted?

andydavison · 2020-07-30T14:06:22Z

@maidbarion one quick thing — I think we mentioned in passing in a chat, but I don't know if it makes sense to number the story files for the components e.g. stories/3-SessionProvider.stories.tsx could just be stories/SessionProvider.stories.tsx.

Just alphabetical would make it easier to find things and we don't have to worry about editing all the other file names to put something in a specific place.

Co-authored-by: Aaron James Madden <aaronjamesmadden@Aarons-MacBook-Pro.local>

maidbarion assigned ajacksified Jul 23, 2020

ajacksified suggested changes Jul 23, 2020

View reviewed changes

andydavison reviewed Jul 24, 2020

View reviewed changes

maidbarion marked this pull request as ready for review July 24, 2020 15:02

ajacksified suggested changes Jul 24, 2020

View reviewed changes

ajacksified suggested changes Jul 27, 2020

View reviewed changes

ajacksified approved these changes Jul 27, 2020

View reviewed changes

vercel Bot had a problem deploying to Preview July 29, 2020 08:50 Failure

vercel Bot had a problem deploying to Preview July 29, 2020 09:10 Failure

vercel Bot had a problem deploying to Preview July 29, 2020 14:42 Failure

vercel Bot had a problem deploying to Preview July 30, 2020 10:32 Failure

ajacksified reviewed Jul 30, 2020

View reviewed changes

Comment thread src/logOut/index.test.tsx

@@ -1,83 +0,0 @@

/**

ajacksified Jul 30, 2020

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Why were these tests deleted?

ajacksified mentioned this pull request Jul 30, 2020

Feature/text component #17

Merged

vercel Bot deployed to Preview July 31, 2020 15:20 View deployment

github-actions Bot deployed to review July 31, 2020 15:20 View deployment

vercel Bot deployed to Preview July 31, 2020 15:26 View deployment

github-actions Bot deployed to review July 31, 2020 15:26 View deployment

vercel Bot deployed to Preview August 3, 2020 08:53 View deployment

github-actions Bot deployed to review August 3, 2020 08:53 View deployment

ajacksified force-pushed the feature/session-provider branch from 9c2b4e5 to e3cb6f5 Compare August 3, 2020 14:51

vercel Bot deployed to Preview August 3, 2020 14:52 View deployment

github-actions Bot deployed to review August 3, 2020 14:52 View deployment

ajacksified force-pushed the feature/session-provider branch from e3cb6f5 to e7f3322 Compare August 3, 2020 14:54

vercel Bot deployed to Preview August 3, 2020 14:54 View deployment

github-actions Bot deployed to review August 3, 2020 14:54 View deployment

SessionProvider component

c55a695

ajacksified force-pushed the feature/session-provider branch from e7f3322 to c55a695 Compare August 3, 2020 14:58

vercel Bot deployed to Preview August 3, 2020 14:58 View deployment

github-actions Bot deployed to review August 3, 2020 14:58 View deployment

maidbarion merged commit 0dd7b5c into master Aug 3, 2020

ajacksified pushed a commit that referenced this pull request Aug 4, 2020

SessionProvider component (#16)

8f71117

Co-authored-by: Aaron James Madden <aaronjamesmadden@Aarons-MacBook-Pro.local>

maidbarion deleted the feature/session-provider branch August 12, 2020 12:18

Uh oh!

Conversation

maidbarion commented Jul 23, 2020

Uh oh!

ajacksified left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andydavison Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maidbarion commented Jul 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vercel Bot commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andydavison commented Jul 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andydavison Jul 24, 2020 •

edited

Loading

vercel Bot commented Jul 29, 2020 •

edited

Loading