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

Feature/html5 autocomplete #146

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

josiahwilliams
Copy link
Collaborator

@josiahwilliams josiahwilliams commented Jun 20, 2024

@josiahwilliams josiahwilliams marked this pull request as draft June 20, 2024 18:18
@alisonhall

This comment was marked as resolved.

@alisonhall
Copy link
Collaborator

The code walkthrough steps currently talk more about how to create a form and all of the attributes, rather than how to include the autocomplete functionality and how it's accessible. The code highlighting of the steps also highlights a lot of extra lines that shouldn't be highlighted. We should probably rethink what steps are actually needed for autocomplete.

@josiahwilliams

This comment was marked as resolved.

@josiahwilliams

This comment was marked as resolved.

@alisonhall

This comment was marked as resolved.

@zoltan-dulac
Copy link
Collaborator

Screenshot 2024-06-21 at 7 27 27 AM

http://localhost:8888/autocomplete.php

Screenshot 2024-06-21 at 7 27 27 AM

http://localhost:8888/autocomplete.php

I really like the layout here. Thank you.

@josiahwilliams

This comment was marked as resolved.

@josiahwilliams

This comment was marked as resolved.

@zoltan-dulac

This comment was marked as resolved.

preflight: false, // disable preflight - prevents Tailwind from resetting all styles
}
}
</script>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to remove tailwind from this PR. The reasons are here.

We can discuss one-on-one if you like, of course.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I will work on removing Tailwinds from this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tailwinds has been removed.

expect(domInfo.hasLabel).toBe(true);
}
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the idea of checking forms with labels. Let's also ensure the autocomplete functionality you have implemented is also checked here as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added tests to test autocomplete functionality.

cc:
@zoltan-dulac

@josiahwilliams

This comment was marked as resolved.

@josiahwilliams josiahwilliams marked this pull request as ready for review June 28, 2024 16:07
@josiahwilliams josiahwilliams changed the title [WIP] Feature/html5 autocomplete Feature/html5 autocomplete Jun 28, 2024
@josiahwilliams
Copy link
Collaborator Author

We definitely need to discuss including tailwind before merging this PR. It makes the code walkthrough experience a lot more bloated and hard to read, and it also changes other styles on the page (like the h1).

Is there any way we can not include tailwind at this time?

Tailwind has been removed and replaced with BEM and LESS.

return document
.querySelector('#first-name')
.getAttribute('autocomplete');
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are checking specific input fields for specific autocomplete values. This works for this case, but how about if I add form fields?

What I would like to do is have an array that contains all the possible form field autocomplete attributes (you can get them on the MDN autocomplete page) and check that all input fields have a valid autocomplete value.

This is much more scalable (e.g. How about if I decide to add another form field?)

givenNameInput.value = 'John';
givenNameInput.dispatchEvent(
new Event('input', { bubbles: true }),
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to test if this stuff. Let's talk about it at standup.

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.

3 participants