-
Notifications
You must be signed in to change notification settings - Fork 28
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
Discussion
as an island
#3839
Discussion
as an island
#3839
Conversation
This change will get overwritten once I merge in main
Size Change: -127 kB (-5%) ✅ Total Size: 2.59 MB
ℹ️ View Unchanged
|
// ************************************************************************** | ||
// beingHydrated? | ||
// We use this prop to solve a problem we have with Storybook. If you remove | ||
// it then the page will render fine on production but our layout stories | ||
// will render two copies of Discussion, side by side. The reason for this | ||
// is because we technically server side render these layout stories on the | ||
// client, inside the story file itself. | ||
// | ||
// Yes, it's true, having props purely to solve test implementation problems | ||
// is not great. If you feel strongly about this and want to remove this | ||
// prop I'm okay with that. If you were able to solve this another way | ||
// then thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋
const { data: user } = useApi<{ userProfile: UserProfile }>( | ||
isSignedIn | ||
? joinUrl( | ||
discussionApiUrl, | ||
'profile/me?strict_sanctions_check=false', | ||
) | ||
: '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does useApi behave when passed an empty string? Does it throw an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed that pattern now and replaced it with composition. I was hoping to avoid using that approach because it's a lot of file renaming but it seems to be the only way. If you pass fetch ''
then it makes a call to the domain the page is on, which isn't what we want.
)} | ||
|
||
{beingHydrated && !isExpanded && ( | ||
<Lazy margin={300} disableFlexStyles={true}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer need this Lazy component because Island provides this deferred loading built in
// prop I'm okay with that. If you were able to solve this another way | ||
// then thank you! | ||
beingHydrated?: boolean; | ||
// ************************************************************************** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prop was needed to fix an issue with hydration in Storybook. This problem was solved by the islands pattern so this prop could be removed.
@@ -108,7 +106,6 @@ let renderCount = 0; | |||
export const App = ({ CAPI, ophanRecord }: Props) => { | |||
log('dotcom', `App.tsx render #${(renderCount += 1)}`); | |||
const isSignedIn = !!getCookie({ name: 'GU_U', shouldMemoize: true }); | |||
const [user, setUser] = useState<UserProfile | null>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR removes the global state of user
from App.tsx
CAPI.config.discussionApiClientHeader | ||
} | ||
enableDiscussionSwitch={ | ||
CAPI.config.switches.enableDiscussionSwitch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now set this property correctly using the actual switch value from CAPI
part of #3629 |
@@ -180,62 +164,29 @@ export const Discussion = ({ | |||
/> | |||
</div> | |||
</Hide> | |||
|
|||
{beingHydrated && isExpanded && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we test what happens with expanded behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to Olly's suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 Would like to know the results of testing isExpanded behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 I think this is a great refactoring
What does this change?
This moves
Discussion
over to the island patternAlong the way
To make this transition work the following refactors and changes were needed
Discussion v9.1.1
It turns out
Discussion
wasn't being included in our automated accessibility checks but that when we loaded it onto the page via an island this issue was fixed. However, this meant we started to see new problems in the report failing the build. To get past this I fixed those issues indiscussion-rendering
and brought that new release in hereNo
user
in App.tsxThis PR removes the global state of
user
from App.tsxNo more
beingHydrated
This prop was needed to fix an issue with hydration in Storybook. This problem was solved by the islands pattern so this prop could be removed.
DiscussionContainer
This is a wrapper component that is being used to compose the decision about if the user is signed in or not. If they are, we render another wrapper component
DiscussionWhenSigned
which is where we fetch the user profile, and if they are not, we directly renderDiscussion
itself.This level of composition is needed because you cannot conditionally call a react hook.
Why?
🏖️ Islands are where we want to be 🏝️