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 experiment about page as Router option #1367

Closed
wants to merge 2 commits into from

Conversation

BeritJanssen
Copy link
Collaborator

This would be an option towards #1337 : the about page can also function as a landing page.

Copy link
Contributor

@drikusroor drikusroor left a comment

Choose a reason for hiding this comment

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

I don't think the proposed code will do what it is supposed to do and my advice would be to improve the logic in the Experiment component (see my comment).

Comment on lines +88 to +90
{/* ExperimentAbout */}
<Route path={URLS.experimentAbout} element={<ExperimentAbout />} />

Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit confused :-) Is this supposed to be an about page for an experiment? In that case, I don't think this route will work properly as there the route / page component won't know for which experiment they will have to show the about page. I think it might be better to modify the Experiment component's logic to decide whether we want to show the consent already if the user wants to go to the experiment's about page.

Either here by checking the current location and see if the current location contains "/about":

if (!hasShownConsent && showConsent) {
const attrs = {
participant,
onNext,
experiment,
...experiment.consent,
}
return (
<DefaultPage className='aha__consent-wrapper' title={experiment.name}>
<Consent {...attrs} />
</DefaultPage>
)
}

Or here, by moving the logic that decides whether we want to show the consent form to a separate "Guard Component" (and something similar for the logic that decides whether participant will be redirected to next experiment instead of the dashboard):

export const RequireConsent = ({ experiment, hasShownConsent, children }: RequireConsentProps) => {
    const showConsent = experiment?.consent;
    
    if (!hasShownConsent && showConsent) {
        return <Navigate to={`/experiment/${experiment.slug}`} replace />;
    }

    return <>{children}</>;
};
export const RequireDashboardOrRedirect = ({ experiment, children }: RequireDashboardOrRedirectProps) => {
    const displayDashboard = experiment?.dashboard.length;
    const nextBlock = experiment?.nextBlock;
    const participantIdUrl = experiment?.participantIdUrl;
    
    if (!displayDashboard && nextBlock) {
        return <Navigate 
            to={`/block/${nextBlock.slug}${participantIdUrl ? `?participant_id=${participantIdUrl}` : ""}`} 
            replace 
        />;
    }

    return <>{children}</>;
};
            <Routes>
                {/* About route is always accessible */}
                <Route
                    path="/about"
                    element={<ExperimentAbout content={experiment?.aboutContent} slug={experiment.slug} />}
                />
                
                {/* Main experiment route with nested guards */}
                <Route
                    path="*"
                    element={
                        <RequireConsent experiment={experiment} hasShownConsent={hasShownConsent}>
                            <RequireDashboardOrRedirect experiment={experiment}>
                                <ExperimentDashboard 
                                    experiment={experiment} 
                                    participantIdUrl={participantIdUrl} 
                                    totalScore={experiment.totalScore} 
                                />
                            </RequireDashboardOrRedirect>
                        </RequireConsent>
                    }
                />
            </Routes>

@BeritJanssen
Copy link
Collaborator Author

BeritJanssen commented Nov 19, 2024

Yup, I rushed this PR and forgot to pass the slug in the Route, silly mistake. When you do though, this (more specific) route {slug}/about should take precedence over the {slug} route, right? I do think it's a good idea to rework the Experiment component and factor out the consent & redirect/dashboard logic into their own components. Can you open an issue for it?

As we now got yet another feature request for a landing page, which cannot be solved with this approach (as it specifies a landing page for two different experiments), closing this PR. I'm now thinking of a dedicated Django landing app and route, and a React Landing component, which would probably be no more than a wrapper around HTML entered from the Django admin.

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