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

Added data-tag attribute to passageCard #1493

Merged
merged 4 commits into from
Feb 11, 2024

Conversation

rubynguyen1510
Copy link
Contributor

Description

  • Added passage tags to passage-card element using data-tags to enhance customizability.
  • Tested the presence of data-tags.
  • Included screenshots of successful execution.

Issues Fixed

#1438

Testing

In the development of our test to validate the data-tags attribute on the passage card, we evaluated two potential selection methods. They were 'getByTestId' and 'getByText'.

  • getByTestID

    const passageElement = screen.getByTestId('passage-card');

  • getByText

    const passageElement = screen.getByText(passage.name).closest('.passage-card');

We chose getByTestId over getByText because it is more reliable, and efficient and could be used to specifically target the passage card element.

Test Results

testRun data-taginHTML

Credit

[X] We would like to be credited in the application as Siham Argaw and Ngoc Nguyen.

Presubmission Checklist

[X] We have read this project's CONTRIBUTING file, and this PR follows the criteria laid out there.
[X] This contribution was created by us and we have the right to submit it under the GPL v3 license. (This is not a rights assignment.)
[X] We have read and agree to abide by this project's Code of Conduct.

Copy link
Owner

@klembot klembot left a comment

Choose a reason for hiding this comment

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

Thanks for your PR! I have some suggestions on revisions. If you have followup questions, feel free to ask.

src/components/passage/__tests__/passage-card.test.tsx Outdated Show resolved Hide resolved
src/components/passage/__tests__/passage-card.test.tsx Outdated Show resolved Hide resolved
src/components/passage/__tests__/passage-card.test.tsx Outdated Show resolved Hide resolved
src/components/passage/__tests__/passage-card.test.tsx Outdated Show resolved Hide resolved
src/components/passage/passage-card.tsx Outdated Show resolved Hide resolved
@klembot
Copy link
Owner

klembot commented Feb 9, 2024

@rubynguyen1510 looks good now, but one last question before I merge: any reason you used data-passage-tag instead of data-passage-tags? We've been generally using the plural in code.

@rubynguyen1510
Copy link
Contributor Author

Thanks for your feedback! We overlooked the plural naming convention. Just fixed it!

@klembot
Copy link
Owner

klembot commented Feb 10, 2024

@rubynguyen1510 almost there, I think!

@klembot
Copy link
Owner

klembot commented Feb 11, 2024

Looks good--thanks for your work!

@klembot klembot merged commit 5e2abc9 into klembot:develop Feb 11, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants