Callouts in DCR - PR 1/5 - Add deadline component and separate new/legacy components into two folders #6741
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current Form.tsx and FileUpload.tsx are soon to be legacy components
tkgnm
commented
Dec 9, 2022
| @@ -0,0 +1,62 @@ | |||
| // File upload is the only legacy component that is still used on the old callout form | |||
There was a problem hiding this comment.
This is the old FileUpload component. It's now going to live in the (legacy) CalloutEmbed folder as it is only used there.
tkgnm
commented
Dec 9, 2022
| @@ -1,31 +1,79 @@ | |||
| import { css } from '@emotion/react'; | |||
There was a problem hiding this comment.
This is the nice shiny new FileUpload component.
tkgnm
commented
Dec 9, 2022
| @@ -3,11 +3,11 @@ import { text, textSans } from '@guardian/source-foundations'; | |||
| import { Button, Link } from '@guardian/source-react-components'; | |||
| import { useState } from 'react'; | |||
| import type { CampaignFieldType } from '../../../types/content'; | |||
| import { MultiSelect } from '../Callout/MultiSelect'; | |||
There was a problem hiding this comment.
All of the other form components (such as the ones imported here) can be shared between both forms, as they are identical in design. Hence, the old CalloutEmbed imports them from the Callout folder.
There was a problem hiding this comment.
Looks like the other components (e.g. CheckboxSelect, FieldLabel, etc.) are currently under the CalloutEmbed folder - is that right? From this comment (and I didn't see anything to the contrary) it sounds like they're to be used by the new Callout component and stored under its folder?
tkgnm
changed the title
| <div css={uploadStyles}> | ||
| <FieldLabel formField={formField} /> | ||
| <div> | ||
| <label css={customUpload(format)} htmlFor={formField.id}> |
There was a problem hiding this comment.
Could we use the FieldLabel here rather than having 2 labels for one component?
There was a problem hiding this comment.
We still do use the FieldLabel on line 97, it's just that for accessibility/CSS reasons we had to wrap the 'Choose File' button in a <label> tag so that the whole form field (including the label) was not clickable. This also enables us to neatly mark what field this button is related to via the htmlFor attribute.
There was a problem hiding this comment.
Is the field label a html label element? If we could wrap the Choose file button in the FieldLabel rather than the label the whole thing would be clickable and there wouldn't be two labels for one button which probably doesn't make sense semantically
There was a problem hiding this comment.
Yep I get what you mean here. The problem I found was that when the whole thing was clickable you had unexpected behaviour when the 'Remove File' button appeared after uploading a file - it would mean that when you click that button you're immediately prompted to upload a file again. Just a weird UX. However, if you think there is a way around this I'd be more than happy to pair on it!
There was a problem hiding this comment.
I think maybe this is all a moot conversation now as we've moved the file input into source. For reference in source the label wraps the Choose file div, and the Remove file button sits outside of the label
There was a problem hiding this comment.
Thanks @sophie-macmillan - I've removed this component now and have pushed the changes.
⚡️ Lighthouse report for the changes in this PRLighthouse tested 2 URLs Report for Article
Report for Front
|
|
Size Change: -9 B (0%) Total Size: 2.29 MB ℹ️ View Unchanged
|
tkgnm
commented
Dec 9, 2022
| const onSelectFile = async (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| if (event.target.files && event.target.files[0]) { | ||
| if (event.target.files?.[0]) { |
There was a problem hiding this comment.
The && operator seemed unnecessary here, hence I removed it
There was a problem hiding this comment.
We even have a proposal to enforce this in the future: #6715
|
Moved back to draft whilst we figure out what caused a bug in a previous PR |
tkgnm
changed the title
abeddow91
approved these changes
Dec 20, 2022
| return (second.getTime() - first.getTime()) / ONE_DAY; | ||
| } | ||
|
|
||
| export const getDeadlineText = ( |
There was a problem hiding this comment.
are we actually exporting this function? if not we can lose the export key word
|
|
||
| function formatOptionalDate(date: number | undefined): Date | undefined { | ||
| if (date === undefined) return undefined; | ||
| const d = new Date(date * 1000); |
There was a problem hiding this comment.
we don't need to store this into a const, we can just return this line 👍
| }; | ||
|
|
||
| export const FileUpload = ({ formField, formData, setFormData }: Props) => { | ||
| const customUpload = (format: ArticleFormat): SerializedStyles => css` | ||
| ${textSans.small()}; |
There was a problem hiding this comment.
presuming we don't need this as we have another text declaration below on line 59
|
|
||
| const deadlineStyles = css` | ||
| ${textSans.xxsmall()}; | ||
| color: ${palette.brand}; |
tkgnm
changed the title
tkgnm
changed the title
This was referenced Dec 23, 2022
tkgnm
changed the title
This was referenced Dec 23, 2022
Georges-GNM
approved these changes
Dec 28, 2022
There was a problem hiding this comment.
This looks good overall!
Aside from the question about which folder some of the components are in, I remember speaking about what to rename the legacy component and saying there was no set rule to my knowledge - in hindsight, I'd say it would be helpful if CalloutEmbed could be called something like LegacyCalloutEmbed to highlight its status.
Thanks for the approval @Georges-GNM - everything in the folder |
tkgnm
deleted the
tom-king-add-custom-upload-make-legacy-callout-folders
branch
This was referenced Jan 4, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Callouts in DCR
This PR is part of a larger body of work to create a new Callout component in DCR. We are separating this PR into chunks so that it is easier to communicate the intent of each PR and to make it easier for DCR to review. After each PR is merged, we can merge it into the next branch which will incrementally reduce the size of each subsequent PR.
Important note for DCR reviewers: Please review these PR's in order below!
PR 1 #6741
PR 2 #6819
PR 3 #6833
PR 4 #6806
PR 5 #6836
Why?
We are soon to support a newer type of callout in DCR. However, as it necessary to still support the older (legacy) callout, we need to keep both in production. This PR is largely a shuffling of files around in the codebase - legacy files are now in a folder named
CalloutEmbed, and files for the new component go into a folder simply namedCallout. This PR also adds adeadlinecomponent that will be used in subsequent PR's that will implement our new callout component.What does this change?
Deadline. This will be used in a subsequent PR when we add our newCalloutBlockComponent.CalloutEmbed. Soon after the rollout of our shiny new callout component, all of these files will be deleted. However, for now it is still necessary to keep them in production in order to support our current callout embed.Testing
This PR should not change anything in production. The new deadline component is not being used anywhere as of yet, but it will be soon. The rest of the PR is just moving and renaming files in the codebase.
Screenshots