Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

refactor(rome_js_analyze): update error messages for UseAltText diagnostics #4440

Merged
merged 3 commits into from
May 8, 2023

Conversation

nissy-dev
Copy link
Contributor

Summary

This PR is a follow-up for #4437
I updated error messages considering object elements.

Test Plan

I updated snapshots.

Changelog

This is internal refactoring.

  • The PR requires a changelog line

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented May 7, 2023

Deploy Preview for docs-rometools canceled.

Name Link
🔨 Latest commit 33e8927
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/6459026ca521b1000804df2f

@github-actions github-actions bot added the A-Linter Area: linter label May 7, 2023
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

There's a small opportunity for refactoring :) overall, it looks good!

Comment on lines 125 to 129
"Provide a text alternative through the "<Emphasis>"title"</Emphasis>", "<Emphasis>"aria-label"</Emphasis>" or "<Emphasis>"aria-labelledby"</Emphasis>" attribute"
).to_owned(),
_ => markup!(
"Provide a text alternative through the "<Emphasis>"alt"</Emphasis>", "<Emphasis>"aria-label"</Emphasis>" or "<Emphasis>"aria-labelledby"</Emphasis>" attribute"
).to_owned(),
Copy link
Contributor

@ematipico ematipico May 8, 2023

Choose a reason for hiding this comment

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

Here's a suggestion, you could DRY this code like this:

First, we apply rome_console::fmt::Display to ValidatedElement:

impl rome_console::fmt::Display for ValidatedElement {
    fn fmt(&self, fmt: &mut Formatter) -> std::io::Result<()> {
        match self {
            ValidatedElement::Object => fmt.write_markup(markup!(<Emphasis>"title"</Emphasis>)),
            _ => fmt.write_markup(markup!(<Emphasis>"alt"</Emphasis>))
        }
    }
}

And then we just need to display it here:

let message = markup!(
"Provide a text alternative through the "{{validate_element}}", "<Emphasis>"aria-label"</Emphasis>" or "<Emphasis>"aria-labelledby"</Emphasis>" attribute"
).to_owned();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I updated in 33e8927

@ematipico ematipico merged commit 6abe553 into main May 8, 2023
@ematipico ematipico deleted the fix/fix-error-msg branch May 8, 2023 15:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants