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-title-name #512

Closed
wants to merge 1 commit into from
Closed

Added-title-name #512

wants to merge 1 commit into from

Conversation

kekubhai
Copy link

@kekubhai kekubhai commented Aug 1, 2024

Issue Title: Title Name not showing for icons

closes #508

Type of change ☑️

What sort of change have you made:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, local variables)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist: ☑️

  • My code follows the guidelines of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly wherever it was hard to understand.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added things that prove my fix is effective or that my feature works.
  • Any dependent changes have been merged and published in downstream modules.

How Has This Been Tested? ⚙️

I have made my changes and tested by running on the localhost server. It is working fine and showing my done relevenat changes as per the given requirement.

Copy link

vercel bot commented Aug 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
draw-it-out ❌ Failed (Inspect) Aug 1, 2024 3:34pm
draw-it-out-zbd1 ❌ Failed (Inspect) Aug 1, 2024 3:34pm

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to Our repository.🎊 Thank you so much for taking the time to point this out.

Comment on lines +175 to 181

>
{darkMode ? (
<FaSun className="text-black" />
<FaSun className="text-black " title="LightMode" />
) : (
<FaMoon className="text-white" />
<FaMoon className="text-white" title="DarkMode"/>
)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi there @kekubhai 👋🏻

The changes you made to improve accessibility is really impressive, but you still need to refactor this part a little bit. So that we can make truly accessible Theme Toggle.

You need to change the div into a button element, Then you need to add aria-label with "Theme Toggle - ${darkMode ? "Light" : "Dark"} Enabled". Now you can remove title from FontAwesome Icons, and add title="Theme Toggler" for the button element itself.

aria-label would help us in assisting accessibility devices, whereas title would provide a visual tooltip (only accessible for user who don't have visual problems).


Additionally, I've seen lot title attributes which are added to interactive elements. There's a thing you need to do, it's nothing much complicated. Just add aria-label attribute (for every elements where add you added title) copy the contents of title for aria-label so that we can assure users who have visual problems, they might rely on screen readers like accessibility devices.

@kekubhai
Copy link
Author

kekubhai commented Aug 4, 2024 via email

@kekubhai
Copy link
Author

kekubhai commented Aug 5, 2024

sorry for the delay,I couln't get time time to perfrom the necessary changes. It would be better if you could assign any other contributer to work on the issue.
Thank you

@0xabdulkhaliq
Copy link
Collaborator

0xabdulkhaliq commented Aug 6, 2024

@kekubhai, Are you sure?

You have already done quite reasonable changes in this PR, for now you just need to refactor the changes you made. (You can refactor within 10 minutes, I already mentioned what needs be refactored).

Decision is yours, If you still willing to not to work on this issue then let me know about it. So that i can close this PR and Assign any other willing person for contribution.

@kekubhai
Copy link
Author

kekubhai commented Aug 6, 2024 via email

@0xabdulkhaliq
Copy link
Collaborator

As you wish @kekubhai 🤝

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants