Add survey footer

[henrycatalinismith]

Old survey footer

The old survey has a footer with some text about Zetkin and some links. It's a minor detail but it's how I personally found my way to Zetkin after joining Vänsterpartiet so it seems worth the effort to port it over to the new survey. Here are some screenshots of that old footer to set up some context for the PR.

Mobile	Desktop

New survey

I've tried to port the old footer over relatively faithfully. Kept the same text and a similar layout. The list of links wrapped a bit unnecessarily in the old one and I haven't kept that one detail.

Mobile	Desktop

As mentioned in the Slack thread I'm happy to work through any amount of feedback on this so whether you just wanna tweak some details or rework it entirely into a global footer I'm game.

[richardolsson]

Awesome! I would love it if this could replace the footer I added in HomeLayout (on main) which would also probably fix the problem described in #2432.

Would you be able to add it there, and also fix some minor things below?

[richardolsson]

Maybe we can call this something more general, e.g. PublicFooter and then we can use it on all public pages?

[henrycatalinismith]

This sounds great. Where do you reckon is a good place for that in terms of the repo's directory structure? I'm perfectly happy to just do this as an in-place rename within src/features/surveys/components/surveyForm but it feels like there's a 50:50 chance you might have a clear home in mind for a general purpose component like this.

[richardolsson]

Great question! Maybe we make it a ZUI component, e.g. in src/zui/ZUIPublicFooter?

[richardolsson]

One idea that I have is that we could maybe add a "This instance of Zetkin is hosted and managed by X" where X is the organization that hosts the instance. Most organizations use our multi-tenant instance, but an increasing number of organizations are requesting to host their own, and this could be a good way to distinguish them.

I was thinking to make the name of the orgnanization ("X") an environment variable, and the rest of the label would be internationalized the normal way. If the environment variable is missing, the label is excluded.

If that environment variable is set, perhaps the footer could also include a link to the organization, which would require a second environment variable.

What do you think?

[henrycatalinismith]

Love the org name & link idea. Have pushed a first draft implementation in 656ac94. Especially happy to rethink the naming decisions in that commit as this was not a detail I sat and philosophised about at any particular length.

Name only

NEXT_PUBLIC_HOSTING_ORGANIZATION_NAME="Svenskt Näringsliv" yarn devserver

Mobile	Desktop

Name and link

NEXT_PUBLIC_HOSTING_ORGANIZATION_NAME="Svenskt Näringsliv" NEXT_PUBLIC_HOSTING_ORGANIZATION_HREF="https://example.org/" yarn devserver

Mobile	Desktop

[richardolsson]

Love the org name & link idea. Have pushed a first draft implementation in 656ac94. Especially happy to rethink the naming decisions in that commit as this was not a detail I sat and philosophised about at any particular length.

Maybe something like INSTANCE_OWNER_NAME and INSTANCE_OWNER_HREF?

I'm also not sure if we need the NEXT_PUBLIC? We shifted away from using that naming convention when we introduced useEnv() but I honestly can't fully remember why we didn't want to use the environment variables directly. It may have been related to typing.

Maybe you have some good knowledge on this topic that could help lead the way here?

[henrycatalinismith]

So in 5087dd5be I've renamed it according to your suggestion but kept the NEXT_PUBLIC_ prefix. Looks like this.

NEXT_PUBLIC_INSTANCE_OWNER_NAME="Svenskt Näringsliv" NEXT_PUBLIC_INSTANCE_OWNER_HREF="https://example.org/" yarn devserver

If I then strip off that NEXT_PUBLIC_ prefix, it ends up like this.

INSTANCE_OWNER_NAME="Svenskt Näringsliv" INSTANCE_OWNER_HREF="https://example.org/" yarn devserver

My understanding of the error is this: the removal of the prefix makes the value inaccessible in the client-side build, so the first client-side render removes the message about the instance owner, which triggers a hydration failure error due to the mismatch between the server-side markup and the client-side markup. Don't really understand the part about Expected server HTML to contain a matching <img> in <div> if I'm being completely honest but apart from that it makes sense to me why removing the prefix causes a problem.

[henrycatalinismith]

It occurred to me after posting the above that your expectation here may have been that <SurveyFooter /> is a server component and thus has no need to make its environment variable dependencies available for client-side renders. Sadly not the case, as it's rendered by <SurveyForm /> which contains 'use client'; at the top.

I think it would be possible to drop this name prefix if the footer could be made a server component. And I think making the footer a server component would be a desirable goal if we're fairly sure it's going to remain a mostly static blob of HTML. Probably possible it could pick up one or two interactive bits and pieces like e.g. a language picker dropdown, but maybe that's it.

Here's a gist showing a quick rough first draft of that change. It didn't work out, due to some kind of error about a createContext call originating in emotion-element.

I think it seems important and worthwhile to solve this because it appears to be a potential general-case blocker for creating server components of certain kinds. Also possible it's becoming a deep enough rabbit hole to be worth investigating in its own right outside the scope of this pull request though.

[henrycatalinismith]

Returning this stream-of-consciousness series of posts back to the main topic though: perhaps from a defensive coding perspective it might be preferable to keep the NEXT_PUBLIC_ prefix on these two values so that it's easy to drop them into a client component some day without having to deal with the headache of orchestrating a backwards incompatible operational change that needs shipping across multiple self-hosted Zetkin instances?

[richardolsson]

My understanding of the error is this: the removal of the prefix makes the value inaccessible in the client-side build, so the first client-side render removes the message about the instance owner, which triggers a hydration failure error due to the mismatch between the server-side markup and the client-side markup. Don't really understand the part about Expected server HTML to contain a matching <img> in <div> if I'm being completely honest but apart from that it makes sense to me why removing the prefix causes a problem.

I should have made myself more clear (and given it some more thought before my response).

We typically don't access environment variables directly in the code (except in code that will only ever be executed on the server). Instead, we use the useEnv() hook which uses context to retrieve (among other things) certain specific variables that are passed to the context from the server. This happens for the app router in the top-level layout.tsx (which is a RSC) and for the pages router in _app.tsx along with scaffold() used in all the pages' getServerSideProps().

I remember now why we did this. We don't want environment variables to be built into the code bundle, because that way they have to be defined at build time. But we build docker images that we want to reuse across instances, meaning that the values can't be known until runtime.

IMO, this is a flaw in the NEXT.js model for environment variables. I understand the desire to protect environment variables, and to be secure by default, but that shouldn't require the variables to be built into the code bundle.

Our solution is also secure by default because no variables are built into the code, and only the selected environment variables will be rendered by the server in a way that make them accessible on the client.

So I think we still need to change this PR to use useEnv().

[henrycatalinismith]

Nice, it was fun to understand this and it gave me some inspiration for a possible future TypeDoc page. I think 6f21047 and d8fad76 capture what you're describing mostly.

I haven't been able to get the links to show up though. I think that's because scaffold() is for the old page router code and no app router equivalent exists yet. Does that sound about right to you? Just asking to try to establish a shared understanding of what the specific next steps here are likely to be.

[richardolsson]

Nice work! I think a typedoc page would probably be super useful. 💯

I haven't been able to get the links to show up though. I think that's because scaffold() is for the old page router code and no app router equivalent exists yet. Does that sound about right to you? Just asking to try to establish a shared understanding of what the specific next steps here are likely to be.

You're right about scaffold(). The app router equivalent (on this topic) is the base layout.tsx, i.e. src/app/layout.tsx, which adds environment variables to the context (but I believe it's missing some).

app.zetkin.org/src/app/layout.tsx

Lines 33 to 37 in 52a330c

	<ClientContext
	envVars={{
	MUIX_LICENSE_KEY: process.env.MUIX_LICENSE_KEY || null,
	ZETKIN_APP_DOMAIN: process.env.ZETKIN_APP_DOMAIN || null,
	}}

[henrycatalinismith]

Lovely, it's working as intended as of 51d688a.

[richardolsson]

Lovely, it's working as intended as of 51d688a.

Perfect! The preview build is failing but that's only because I reconfigured the build according to #2450.

The only thing remaining now is moving the component (and messages) to ZUI. Could you? 🙏