Implement site settings #3071
Implement site settings #3071
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3071 +/- ##
=======================================
Coverage 89.84% 89.84%
=======================================
Files 208 208
Lines 11189 11189
Branches 1106 1106
=======================================
Hits 10053 10053
Misses 802 802
Partials 334 334
Continue to review full report at Codecov.
|
dominik-zeglen
force-pushed
the
tsx/implement-site-settings
branch
from
bb69236
to
64c51ce
Compare
| <SingleSelectField | ||
| choices={Object.keys(translatedAuthorizationKeyTypes()).map( | ||
| key => ({ | ||
| label: translatedAuthorizationKeyTypes()[key], |
There was a problem hiding this comment.
This will evaluate translatedAuthorizationKeyTypes in each iteration of the map loop.
| "&:last-child": { | ||
| paddingRight: 0 | ||
| }, | ||
| width: 48 + theme.spacing.unit / 2 |
There was a problem hiding this comment.
Please name your magic numbers or, even better, extract them from the theme where available.
There was a problem hiding this comment.
And magic number here is 48?
| }; | ||
| const handleSiteSettingsSuccess = (data: ShopSettingsUpdate) => { | ||
| if ( | ||
| !maybe(() => data.shopDomainUpdate.errors.length) && |
There was a problem hiding this comment.
Using maybe in a conditional can lead to unexpected results.
There was a problem hiding this comment.
There are only 3 possibilities here:
errors === null=>falseerrors.length === 0=>falseerrors.length > 0=>true
So it's not like there are N possible results and we can't list them.
| <Card> | ||
| <CardTitle | ||
| title={i18n.t("General Information", { | ||
| context: "card title" |
There was a problem hiding this comment.
Better context: "store configuration". Physical placement (if important) could be part of a translator comment (not possible until we switch to a different translation API).
| fullWidth | ||
| name="name" | ||
| label={i18n.t("Store Name", { | ||
| context: "field label" |
| helperText={ | ||
| errors.name || | ||
| i18n.t("Store Name is shown on taskbar in web browser", { | ||
| context: "help text" |
| })} | ||
| helperText={ | ||
| errors.name || | ||
| i18n.t("Store Name is shown on taskbar in web browser", { |
There was a problem hiding this comment.
What does that mean? Isn't the taskbar the thing at the bottom of Windows?
| error={!!errors.name} | ||
| fullWidth | ||
| name="name" | ||
| label={i18n.t("Store Name", { |
There was a problem hiding this comment.
"Store name" is not a great label as it can be read as "save the name" (including by translators), something more friendly like "Name of your store" or "Your brand name" could work better.
| })} | ||
| helperText={ | ||
| errors.domain || | ||
| i18n.t("Domain is your store URL", { |
There was a problem hiding this comment.
Why don't we give the field a better label? Currently it reads like this:
Whatchamacallit: [_______________]
- Whatchamacallit is your online store's full URL, eg. https://mystore.example.com/
There was a problem hiding this comment.
Maybe something like "URL of your online store"?
| key={maybe(() => key.name)} | ||
| > | ||
| <TableCell> | ||
| {maybe(() => key.name) ? keyTypes[key.name] : <Skeleton />} |
There was a problem hiding this comment.
Please don't use maybe in conditionals.
| <div className={classes.root}> | ||
| <Typography variant="title"> | ||
| {i18n.t("Site Settings", { | ||
| context: "section name" |
dominik-zeglen
force-pushed
the
tsx/implement-site-settings
branch
from
fc533ca
to
258362e
Compare
Resolves #2589
Resolves #3090
Resolves #3089