Shankiyani/initial popup

[shankiyani]

This is the initial change to support a UI for JVS. A go template is used to display a form. On submission of this form, a basic validation is made against the input. If the user provides invalid data, an error message is shown and the user's inputs are maintained. Upon successful submission a script is executed to postMessage back to the target origin that triggered the initial call. This origin must exist within an allow list provided as an env variable.

[sethvargo]

A few things:

• Should we have a more advanced stylesheet framework (e.g. scss or sass)? If not sass or scss, we should use CSS variables for things like colors and border radii.

• Let's declare our browser support matrix for Bets Platform somewhere? I can be the approver. We don't need to support IE 7, but we should be forward about which browsers we're going to support. We cannot assume Chrome for all Bets.

• Always prefer rem to px for responsive designs.

• Prefer flexbox to float.

• Use class and id selectors instead of styling elements directly. It provides more flexibility in the future.

I didn't review the Go code, but there's a lot of nuance in getting form parsing correct (and secure). Let me know when you want a review of the Go code.

[verbanicm]

Should we have a more advanced stylesheet framework (e.g. scss or sass)? If not sass or scss, we should use CSS variables for things like colors and border radii.

I would rather use a frontend framework, otherwise I would prefer to use CSS variables for this use case.

Let's declare our browser support matrix for Bets Platform somewhere? I can be the approver. We don't need to support IE 7, but we should be forward about which browsers we're going to support. We cannot assume Chrome for all Bets.

Always prefer rem to px for responsive designs.

Prefer flexbox to float.

Use class and id selectors instead of styling elements directly. It provides more flexibility in the future.

Yes, all of this, we should probably create a doc for browser support and best practices for CSS/HTML.

This also leads into the frontend framework vs go template discussion and it will probably come down to use case.

[sethvargo]

Should we have a more advanced stylesheet framework (e.g. scss or sass)? If not sass or scss, we should use CSS variables for things like colors and border radii.

I would rather use a frontend framework, otherwise I would prefer to use CSS variables for this use case.

CSS variables are fine with me

Let's declare our browser support matrix for Bets Platform somewhere? I can be the approver. We don't need to support IE 7, but we should be forward about which browsers we're going to support. We cannot assume Chrome for all Bets.

Always prefer rem to px for responsive designs.

Prefer flexbox to float.

Use class and id selectors instead of styling elements directly. It provides more flexibility in the future.

Yes, all of this, we should probably create a doc for browser support and best practices for CSS/HTML.

This also leads into the frontend framework vs go template discussion and it will probably come down to use case.

Yes, I agree we need a doc 😄

[shankiyani]

Thanks for the feedback so far @verbanicm, @sethvargo, and @yolocs. I did some refactoring to mimic the EN service so I hope its more aligned with expectations (still more improvements in upcoming PRs). I wonder if this is a good point to try to merge what we have so far. I want to follow up with the Remaining tasks in the next PR(s). Ill tackle Followups after that.

Remaining:

• Setup IAP to get the authenticated user
• Call the validation service and send the token back (currently hard coding a token which is being sent back to the calling application)
• Write a walkthrough for a client library
• Server side callback flow using a custom header

Followups:

• Browser support matrix doc
• CSS linter in the pkg repo
• Better styling of the alert messages
• More robust testing around the handler, maybe rework the current implementation to reflect how EN does forms (which also handles alert messages).

[shankiyani]

A few things:

• Should we have a more advanced stylesheet framework (e.g. scss or sass)? If not sass or scss, we should use CSS variables for things like colors and border radii.
• [followup] Let's declare our browser support matrix for Bets Platform somewhere? I can be the approver. We don't need to support IE 7, but we should be forward about which browsers we're going to support. We cannot assume Chrome for all Bets.
• Always prefer rem to px for responsive designs.
• Prefer flexbox to float.
• Use class and id selectors instead of styling elements directly. It provides more flexibility in the future.

I didn't review the Go code, but there's a lot of nuance in getting form parsing correct (and secure). Let me know when you want a review of the Go code.

All are either addressed or noted as followups

[sethvargo]

Nit, can be a follow-up. Always prefer document.querySelector.

[sethvargo]

If originElement or windowElement are null, this will NPE.

[sethvargo]

This flow is a bit weird. I think it'd be better to always return short:

Suggested change

	if (!targetOrigin) {
	alert("You must pass a target origin from your application to successfully retrieve a token.")
	window.close()
	} else if (!windowName) {
	alert("You must pass a window name from your application to successfully retrieve a token.")
	} else if (!token) {
	alert("Something went wrong, unable to retrieve a token.")
	} else {
	window.opener.postMessage(
	JSON.stringify({
	// notify the requestor of the window name that was provided,
	// client should check this as a sanity check
	source: windowName,
	payload: {
	token,
	},
	}),
	targetOrigin
	);
	}
	window.close();
	if (!targetOrigin) {
	alert("You must pass a target origin from your application to successfully retrieve a token.")
	window.close()
	return
	}
	if (!windowName) {
	alert("You must pass a window name from your application to successfully retrieve a token.")
	window.close()
	return
	}
	if (!token) {
	alert("Something went wrong, unable to retrieve a token.")
	window.close()
	return
	}
	window.opener.postMessage(
	JSON.stringify({
	// notify the requestor of the window name that was provided,
	// client should check this as a sanity check
	source: windowName,
	payload: {
	token,
	},
	}),
	targetOrigin
	);
	window.close();

This can be a follow-up fix.

[sethvargo]

Style nit: can be a follow-up (throughout all tests, just marking once here):

Suggested change

	for _, tc := range cases {
	tc := tc
	t.Run(tc.name, func(t *testing.T) {
	t.Parallel()
	lookuper := envconfig.MapLookuper(tc.envs)
	for _, tc := range cases {
	tc := tc
	t.Run(tc.name, func(t *testing.T) {
	t.Parallel()
	lookuper := envconfig.MapLookuper(tc.envs)

[sethvargo]

Nit: prefer any over interface{} in modern Go. This can be a follow-up.

[sethvargo]

In general, I would expect all "setup" to be in the controller. So the allowlist would be passed to controller.New. It's not a huge deal and can be a follow-up.