abcxyz / jvs

Apache License 2.0
8 stars 0 forks source link

Shankiyani/initial popup #181

Closed shankiyani closed 1 year ago

shankiyani commented 1 year ago

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.

verbanicm commented 1 year ago

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 commented 1 year ago

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 :smile:

shankiyani commented 1 year ago

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:

Followups:

shankiyani commented 1 year ago

A few things:

  • [x] 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.
  • [x] Always prefer rem to px for responsive designs.
  • [x] Prefer flexbox to float.
  • [x] 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