Skip to content
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

Set focus on the ManageList's inputs when users navigate to them using the List's buttons #78

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vivitt
Copy link
Collaborator

@vivitt vivitt commented Apr 19, 2024

Description

We added buttons below the filter input to enhance app navigation and provide users with a more intuitive way to access the 'Add Items' or 'Share List' forms.

To make the user experience smoother and improve accessibility, this PR adds an optional parameter to the manage-list route. This parameter is used to programmatically set focus on the inputs when users navigate to them using any of the buttons in the List view to add an item or share a list.

Additionally, this change removes the 'selected' property from the first option of the 'Select time' input to avoid a console warning.

After applying these changes:

  • When users navigate to “Manage List” view trough the “Add item” button, the “Add item” form is focused
  • When users navigate to “Manage List” view trough the “Share list” button, the “Share list” form is focused
  • There are no warnings displaying in the console

Type of Changes

Type
✨ New feature
🎨 Enhancement
🐛 Bug fix

Updates

Before

Navigation

Screen.Recording.2024-04-19.at.15.27.14.mov

Warning

Screenshot 2024-04-19 at 15 25 42

After

Navigation

Screen.Recording.2024-04-19.at.15.26.31.mov

Warning

Screenshot 2024-04-19 at 15 26 03

Testing Steps / QA Criteria

To test changes locally, pull the set-focus branch from this repo:

  • git checkout -b set-focus
  • git pull origin set-focus
  • Navigate to the Manage List view using the "Add item" and "Share list" buttons in the List view

- Add an optional parameter in the managelist route to use it to
programatically set focus on the inputs when users navigate to it
using the buttons in the List view.

- Remove selected property in the Select input first's option to
avoid console warning
@vivitt vivitt self-assigned this Apr 19, 2024
Copy link

Visit the preview URL for this PR (updated for commit 5fc3dd0):

https://tcl-71-smart-shopping-list--pr78-set-focus-e6j0myx6.web.app

(expires Fri, 26 Apr 2024 13:37:05 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 1e7ade9d0f374c4ddb5d7ab6fc541062fc7a1ab4

Copy link
Collaborator

@BikeMouse BikeMouse left a comment

Choose a reason for hiding this comment

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

Thank you very much for this! Great work there :)

Copy link
Collaborator

@ocsiddisco ocsiddisco left a comment

Choose a reason for hiding this comment

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

Thank you for the update Viviana! It is a nice add!
As mentioned on Slack, there is only the part regarding the option in select disappearing that bothers me, but it can be improved with 'required' on select and empty value for the option.

Copy link
Collaborator

@borjaMarti borjaMarti left a comment

Choose a reason for hiding this comment

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

Hey @vivitt! Awesome, works perfectly and I like the use of params for the functionality. Left a couple comments marking a leftover console.log and missing dependencies on the useEffect. Thank you!!

@@ -199,6 +203,8 @@ export async function deleteList(userId, userEmail, listPath, listId) {
// Delete list doc
const listCollectionRef = collection(db, userId);
const listDocumentRef = doc(listCollectionRef, listId);
const itemsCollectionRef = collection(listDocumentRef, 'items');
console.log(itemsCollectionRef);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Leftover console.log ^^

} else if (param === Params.Share) {
shareListInput.current.focus();
}
}, []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

34:5 warning React Hook useEffect has a missing dependency: 'param'. Either include it or remove the dependency array react-hooks/exhaustive-deps

Getting this warning, missing param on the dependency array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants