-
Notifications
You must be signed in to change notification settings - Fork 1
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
Enable shopping list title to be edited from shopping lists page #40
Enable shopping list title to be edited from shopping lists page #40
Conversation
…ponent to show and hide the edit form at appropriate sizes
} | ||
|
||
describe('ListEditForm', () => { | ||
test.skip('displays the title and submit button', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is being skipped because rendering the form in JSDOM involves using the node-canvas package, which doesn't support worker_threads. What are worker_threads
and are we using them in this application? Apparently we are. The maintainers have said they would like to add support this year, so I'm skipping this test rather than deleting it entirely.
useEffect(() => { | ||
if (!canEdit || !size || !iconsRef.current) return | ||
|
||
const width = size.width - iconsRef.current.offsetWidth + 16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
16px is the amount of margin on the icons.
const useComponentVisible = () => { | ||
const [isComponentVisible, setIsComponentVisible] = useState(false) | ||
const componentRef = useRef<any>(null) | ||
const triggerRef = useRef<HTMLButtonElement>(null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to be more specific - the trigger should ideally be a button so there's nothing wrong with specifying that here. If we need it changed in the future it's a small enough change then. As for componentRef
on line 5, that got complicated with TypeScript since the ListEditForm
component demanded a particular ref type and this required something different. So we changed both to use RefObject<any>
or LegacyRef<any>
types instead of specifying a particular HTML element.
title, | ||
onSubmit, | ||
}: EditFormProps) => { | ||
const MAX_TEXT_WIDTH = maxTotalWidth - FIXED_BUTTON_WIDTH - 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why - 2
? Not really sure.
Context
We are re-implementing the shopping lists page. The next step is to enable the titles of shopping lists to be edited from the dashboard.
Changes
ListEditForm
component enabling shopping list titles to be edited in placesimApi
andShoppingListsContext
to update shopping list titlesListEditForm
into theShoppingList
componentConsiderations
The auto-sizing nature of the
ListEditForm
component made it a beast to implement now as the first time. Although there were some issues with the form running off the shopping list component, they mostly occurred randomly while changing the viewport size and as such are not a huge concern.Manual Test Cases
All test cases assume you are logged into the front end and on your shopping lists page. The selected game should have at least one editable shopping list.
Cancel Editing by Clicking Outside
Cancel Editing by Pressing Escape
Updating the Title Successfully
Changing to a Default Title
404 Error
ShoppingListsController::UpdateService#perform
method422 Error
500 Error
ShoppingListsController::UpdateService#perform
methodScreenshots and GIFs
GIFs are only provided of certain viewport sizes for file size reasons
320px
481px
601px
769px
1025px
1201px
Large Desktop