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

Remove remaining <br /> #18444

Merged
merged 12 commits into from
Apr 17, 2024
Merged

Remove remaining <br /> #18444

merged 12 commits into from
Apr 17, 2024

Conversation

sergiou87
Copy link
Member

@sergiou87 sergiou87 commented Apr 10, 2024

Description

This PR is a continuation of #18436 , going through every remaining instances of <br /> found in our codebase and replacing them with the appropriate HTML elements.

There have been changes in these components:

  • Large text diffs info text
  • Force-push warning in the pull/push/fetch dropdown
  • Warning in the "enable notifications" setting hint
  • Content of the warning dialog of local changes before undoing
  • Disclaimer footnotes in the welcome start screen
  • Contents of the "add SSH host" dialog
  • Contents of the dialog to confirm removing a repository
  • Zero case of branch selector in the Preview Pull Request dialog
  • Path error message in the "add existing repo" dialog
  • Label of the repository URL textbox in the clone dialog (URL tab)

Only these instances have been left:

Screenshots

Everything should still look the same!

Release notes

Notes: [Fixed] Remove unnecessary br elements and make the app easier to navigate with screen readers

@sergiou87 sergiou87 marked this pull request as ready for review April 12, 2024 11:25
@sergiou87 sergiou87 requested a review from tidy-dev April 12, 2024 11:26
@@ -179,9 +179,8 @@ export class Diff extends React.Component<IDiffProps, IDiffState> {
return (
<div className="panel empty large-diff">
<img src={NoDiffImage} className="blankslate-image" alt="" />
<p>The diff is too large to be displayed by default.</p>
Copy link
Contributor

@tidy-dev tidy-dev Apr 15, 2024

Choose a reason for hiding this comment

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

Should we try to keep it visually as it was before?

Before
CleanShot 2024-04-15 at 14 26 42

After
Large text diffs with extra space.

From accessibility audit documentation, I have branch https://github.com/githuba11y/repo_the_second2/tree/with-github-desktop-readme.md) for reproducing this screen handy.

Comment on lines +72 to +77
<div className="description">
<p>The repository will be removed from GitHub Desktop:</p>
<p>
<Ref>{this.props.repository.path}</Ref>
</p>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Before:
CleanShot 2024-04-15 at 14 39 24

After:
CleanShot 2024-04-15 at 14 38 14

Copy link
Contributor

@tidy-dev tidy-dev left a comment

Choose a reason for hiding this comment

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

You can see in the comments I noticed a couple places where these changes introduced additional whitespace. Other than that, everything looks good. Thank you for going through all these places

@sergiou87
Copy link
Member Author

Thanks for your feedback! Hopefully all fixed now:

image image

@sergiou87 sergiou87 requested a review from tidy-dev April 16, 2024 15:06
Copy link
Contributor

@tidy-dev tidy-dev 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 those changes. This is a great proactive PR. 💖

@sergiou87 sergiou87 merged commit 92fa649 into development Apr 17, 2024
7 checks passed
@sergiou87 sergiou87 deleted the everything-goes-brrr branch April 17, 2024 10:34
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.

2 participants