Skip to content

Add Service Banners #5272

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

Merged
merged 20 commits into from
Dec 6, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Begin integrating color picker
  • Loading branch information
ammario committed Dec 3, 2022
commit d3a4485a57e73949072d04d196f81a64414d8bdf
2 changes: 2 additions & 0 deletions site/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
"@material-ui/icons": "4.5.1",
"@material-ui/lab": "4.0.0-alpha.42",
"@testing-library/react-hooks": "8.0.1",
"@types/react-color": "^3.0.6",
"@vitejs/plugin-react": "2.1.0",
"@xstate/inspect": "0.6.5",
"@xstate/react": "3.0.1",
Expand All @@ -54,6 +55,7 @@
"just-debounce-it": "3.1.1",
"react": "18.2.0",
"react-chartjs-2": "4.3.1",
"react-color": "^2.19.3",
"react-dom": "18.2.0",
"react-helmet-async": "1.3.0",
"react-i18next": "12.0.0",
Expand Down
7 changes: 7 additions & 0 deletions site/src/api/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -668,3 +668,10 @@ export const getServiceBanner = async (): Promise<TypesGen.ServiceBanner> => {
const response = await axios.get(`/api/v2/service-banner`)
return response.data
}

export const setServiceBanner = async (
b: TypesGen.ServiceBanner,
): Promise<TypesGen.ServiceBanner> => {
const response = await axios.put(`/api/v2/service-banner`, b)
return response.data
}
106 changes: 77 additions & 29 deletions site/src/pages/DeploySettingsPage/ServiceBannerSettingsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,14 @@ import { FeatureNames } from "api/types"
import {
Badges,
DisabledBadge,
EnabledBadge,
EnterpriseBadge,
EntitledBadge,
} from "components/DeploySettingsLayout/Badges"
import { Header } from "components/DeploySettingsLayout/Header"
import { LoadingButton } from "components/LoadingButton/LoadingButton"
import { Stack } from "components/Stack/Stack"
import { FormikContextType, useFormik } from "formik"
import React, { useContext } from "react"
import React, { useContext, useState } from "react"
import { Helmet } from "react-helmet-async"
import { pageTitle } from "util/page"
import * as Yup from "yup"
Expand All @@ -21,17 +20,23 @@ import { getFormHelpers } from "util/formUtils"
import makeStyles from "@material-ui/core/styles/makeStyles"
import FormControlLabel from "@material-ui/core/FormControlLabel"
import Switch from "@material-ui/core/Switch"
import { BlockPicker } from "react-color"
import { useTheme } from "@material-ui/core/styles"

import { colors } from "theme/colors"

export const Language = {
messageLabel: "Message",
backgroundColorLabel: "Background Color",
updateBanner: "Update",
previewBanner: "Preview",
}
Copy link
Member

Choose a reason for hiding this comment

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

We're no longer using the Language convention because it isn't conducive to translation and also gets stale without warning. Do you mind updating this PR to use react-i18next?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. What do you by "gets stale without warning"?

Copy link
Member Author

Choose a reason for hiding this comment

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

The translation aspect I've never understood, because it's unlikely we'd translate the product for years.

Copy link
Member

Choose a reason for hiding this comment

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

@ammario, the Language objects are just POJOs so no warning is thrown if we cease to use a string in our JSX. Thus you can have key-value pairs that aren't used, and this was happening more frequently as we grew the code base and refactored.

Regarding translations in general: I don't think we need translations, but I didn't introduce them; we had them when I arrived in the form of these Language objects. I just figured if we were going to bother with translating at all, we might as well use a very popular and robust solution versus something home-baked that wasn't maintainable.

There are a couple of other reasons we might want to use i18next: it's really nice to have strings all colocated in one area so that we can make sure the verbiage we use is consistent. Libraries like i18next come with features that handle quirks like pluralization and interpolation - thus we don't have to have giant if-blocks in our JSX. Lastly, although the day may be far in the future, if we ever want to translate it's nice to start now when our code base is relatively small and simple.

My two cents! Happy to discuss further online if you'd like. Also happy to consider just putting strings directly in the JSX. I'd just rather not use the Language objects.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ammario, the Language objects are just POJOs so no warning is thrown if we cease to use a string in our JSX. Thus you can have key-value pairs that aren't used, and this was happening more frequently as we grew the code base and refactored.

Makes sense

Regarding translations in general: I don't think we need translations, but I didn't introduce them; we had them when I arrived in the form of these Language objects. I just figured if we were going to bother with translating at all, we might as well use a very popular and robust solution versus something home-baked that wasn't maintainable.

My understanding was that we began using Language objects to aid in testing, with translations as a secondary goal?

There are a couple of other reasons we might want to use i18next: it's really nice to have strings all colocated in one area so that we can make sure the verbiage we use is consistent. Libraries like i18next come with features that handle quirks like pluralization and interpolation - thus we don't have to have giant if-blocks in our JSX. Lastly, although the day may be far in the future, if we ever want to translate it's nice to start now when our code base is relatively small and simple.

My two cents! Happy to discuss further online if you'd like. Also happy to consider just putting strings directly in the JSX. I'd just rather not use the Language objects.

Thank you for sharing, I see those benefits. I struggle with the react-i18next developer experience. Working with JSON is not fun, not conducive to auto-complete, and lacks type errors. There are also many places where we want to intersperse a link or other HTML element into the language, and I don't see a clean way of doing that.


In this PR I have transitioned some of the strings to use translations to be consistent with the rest of the codebase.

Copy link
Member

Choose a reason for hiding this comment

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

@ammario I was told the Language blocks were a way to get started on translations without committing to a library. They're actually kind of annoying to use in tests as if you pull in different components, you have to alias each Language block separately, but maybe there's a benefit I'm overlooking.

To add HTML or links, you'll want to use the library's Trans component. Mathias recently merged up a great example, here. I'll always agree that i18n is more of a hassle than straight JSX. If it feels particularly painful, we can certainly take it out, but should probably discuss in FE variety first; feel free to throw something on the agenda if you'd like!


export interface ServiceBannerFormValues {
message?: string
backgroundColor?: string
enabled?: boolean
preview: boolean
}

// TODO:
Expand Down Expand Up @@ -59,20 +64,19 @@ const ServiceBannerSettingsPage: React.FC = () => {

const onSubmit = (values: ServiceBannerFormValues) => {
const newBanner = {
...serviceBanner,
}
if (values.message !== undefined) {
newBanner.message = values.message
message: values.message,
enabled: true,
background_color: values.backgroundColor,
}
if (values.enabled !== undefined) {
newBanner.enabled = values.enabled
if (values.preview) {
serviceBannerSend({
type: "SET_PREVIEW_BANNER",
serviceBanner: newBanner,
})
return
}
if (values.backgroundColor !== undefined) {
newBanner.background_color = values.backgroundColor
}

serviceBannerSend({
type: "SET_PREVIEW",
type: "SET_BANNER",
serviceBanner: newBanner,
})
}
Expand All @@ -81,6 +85,7 @@ const ServiceBannerSettingsPage: React.FC = () => {
message: serviceBanner.message,
enabled: serviceBanner.enabled,
backgroundColor: serviceBanner.background_color,
preview: false,
}

const form: FormikContextType<ServiceBannerFormValues> =
Expand All @@ -91,6 +96,12 @@ const ServiceBannerSettingsPage: React.FC = () => {
})
const getFieldHelpers = getFormHelpers<ServiceBannerFormValues>(form)

const [backgroundColor, setBackgroundColor] = useState(
form.values.backgroundColor,
)

const theme = useTheme()

return (
<>
<Helmet>
Expand All @@ -107,16 +118,12 @@ const ServiceBannerSettingsPage: React.FC = () => {
<EnterpriseBadge />
</Badges>

<form
className={styles.form}
onSubmit={form.handleSubmit}
// onChange={form.handleSubmit}
>
<form className={styles.form}>
<Stack>
<FormControlLabel
value="enable"
value="enabled"
control={<Switch {...getFieldHelpers("enabled")} color="primary" />}
label="Enable"
label="Enabled"
/>
<TextField
fullWidth
Expand All @@ -125,15 +132,56 @@ const ServiceBannerSettingsPage: React.FC = () => {
variant="outlined"
/>

<LoadingButton
loading={false}
// aria-disabled={!editable}
// disabled={!editable}
type="submit"
variant="contained"
>
{Language.updateBanner}
</LoadingButton>
<Stack>
<h3>Background Color</h3>
<BlockPicker
color={backgroundColor}
onChange={(color) => {
setBackgroundColor(color.hex)
form.setFieldValue("backgroundColor", color.hex)
}}
triangle="hide"
styles={{
default: {
input: {
color: "white",
backgroundColor: theme.palette.background.default,
},
body: {
backgroundColor: "black",
color: "white",
},
},
}}
/>
</Stack>

<Stack direction="row">
<LoadingButton
loading={false}
// aria-disabled={!editable}
// disabled={!editable}
onClick={() => {
form.setFieldValue("preview", true)
onSubmit(form.values)
}}
variant="contained"
>
{Language.previewBanner}
</LoadingButton>
<LoadingButton
loading={false}
// aria-disabled={!editable}
// disabled={!editable}
onClick={() => {
form.setFieldValue("preview", false)
onSubmit(form.values)
}}
variant="contained"
>
{Language.updateBanner}
</LoadingButton>
</Stack>
</Stack>
</form>
</>
Expand Down
41 changes: 37 additions & 4 deletions site/src/xServices/serviceBanner/serviceBannerXService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,22 @@ import { ServiceBanner } from "../../api/typesGenerated"

export const Language = {
getServiceBannerError: "Error getting service banner.",
setServiceBannerError: "Error setting service banner.",
}

export type ServiceBannerContext = {
serviceBanner: ServiceBanner
getServiceBannerError?: Error | unknown
setServiceBannerError?: Error | unknown
preview: boolean
}

export type ServiceBannerEvent =
| {
type: "GET_BANNER"
}
| { type: "SET_PREVIEW"; serviceBanner: ServiceBanner }
| { type: "SET_PREVIEW_BANNER"; serviceBanner: ServiceBanner }
| { type: "SET_BANNER"; serviceBanner: ServiceBanner }

const emptyBanner = {
enabled: false,
Expand All @@ -34,6 +37,9 @@ export const serviceBannerMachine = createMachine(
getServiceBanner: {
data: {} as ServiceBanner,
},
setServiceBanner: {
data: {},
},
},
},
context: {
Expand All @@ -45,7 +51,8 @@ export const serviceBannerMachine = createMachine(
idle: {
on: {
GET_BANNER: "gettingBanner",
SET_PREVIEW: "settingPreview",
SET_PREVIEW_BANNER: "settingPreviewBanner",
SET_BANNER: "settingBanner",
},
},
gettingBanner: {
Expand All @@ -63,12 +70,30 @@ export const serviceBannerMachine = createMachine(
},
},
},
settingPreview: {
entry: ["clearGetBannerError", "assignPreviewBanner"],
settingPreviewBanner: {
entry: [
"clearGetBannerError",
"clearSetBannerError",
"assignPreviewBanner",
],
always: {
target: "idle",
},
},
settingBanner: {
entry: "clearSetBannerError",
invoke: {
id: "setBanner",
src: "setBanner",
onDone: {
target: "gettingBanner",
},
onError: {
target: "idle",
actions: ["assignSetBannerError"],
},
},
},
},
},
{
Expand All @@ -81,16 +106,24 @@ export const serviceBannerMachine = createMachine(
}),
assignBanner: assign({
serviceBanner: (_, event) => event.data as ServiceBanner,
preview: (_, __) => false,
}),
assignGetBannerError: assign({
getServiceBannerError: (_, event) => event.data,
}),
clearGetBannerError: assign({
getServiceBannerError: (_) => undefined,
}),
assignSetBannerError: assign({
setServiceBannerError: (_, event) => event.data,
}),
clearSetBannerError: assign({
setServiceBannerError: (_) => undefined,
}),
},
services: {
getBanner: API.getServiceBanner,
setBanner: (_, event) => API.setServiceBanner(event.serviceBanner),
},
},
)
Loading