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

Show short description in alttxt pane by default #356

Merged
merged 5 commits into from
Apr 23, 2024
Merged

Conversation

NateLanza
Copy link
Contributor

Does this PR close any open issues?

Closes #340

Give a longer description of what this PR addresses and why it's needed

Previously, only the long description was shown in the alttxt pane. This shows the short description by default with a "Show More" button to view the long description. A "Show Less" button allows for returning to the short description.

Provide pictures/videos of the behavior before and after these changes (optional)

Before:
340-before
After:
340-after

Have you added or updated relevant tests?

  • Yes
  • No changes are needed

Have you added or updated relevant documentation?

  • Yes
  • No changes are needed

Are there any additional TODOs before this PR is ready to go?

TODOs:

  • ...

Copy link

netlify bot commented Apr 22, 2024

Deploy Preview for upset2 ready!

Name Link
🔨 Latest commit 8579c73
🔍 Latest deploy log https://app.netlify.com/sites/upset2/deploys/6627025927715700083b7599
😎 Deploy Preview https://deploy-preview-356--upset2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@NateLanza NateLanza requested a review from JakeWags April 22, 2024 21:24
Copy link
Member

@JakeWags JakeWags left a comment

Choose a reason for hiding this comment

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

Overall looks good, although I'm unsure about converting an error message into an AltText object. That feel's pretty hacky. Please review that and see if you can find a better way to handle those errors.

* @param err - The error message to convert.
* @returns The AltText object with the error message as the long description, short description, and technique description.
*/
function errToAltText(err: string): AltText {
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning behind this? Could you throw an error and catch it in the calling function instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. This should be addressed now; I'm using errors instead. It's also hiding the "Show More" button when an error occurs.

@NateLanza NateLanza requested a review from JakeWags April 23, 2024 00:34
Copy link
Member

@JakeWags JakeWags left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for changing that 👍 Approved

@NateLanza NateLanza merged commit def4a45 into main Apr 23, 2024
7 checks passed
@NateLanza NateLanza deleted the 340-shortdesc branch April 23, 2024 20:56
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.

Short description alt text
2 participants