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

Class diagram accessibility #2911

Merged
merged 8 commits into from
Apr 7, 2022
Merged

Class diagram accessibility #2911

merged 8 commits into from
Apr 7, 2022

Conversation

gwincr11
Copy link
Contributor

@gwincr11 gwincr11 commented Apr 7, 2022

📑 Summary

This adds titles to class diagrams.

📏 Design Decisions

Following the already set pattern for adding accessibility to charts, setup in #2732

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added unit/e2e tests (if appropriate)
  • 🔖 targeted develop branch

@@ -345,8 +345,34 @@ const setDirection = (dir) => {
direction = dir;
};

let title = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to add the default "Class chart" title text here in case a user doesn't pass in anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am on the fence, if the title starts getting shown that is not ideal... Unless it gets the user to add a better title. I have been back and forth on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do think there should be some work to get titles to show up on every chart. Perhaps this is a carrot to making things more accessible?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhhh right - I forgot that this would be shown to all users 😬 Let's skip for now, but maybe we can revisit the option to have a title displayed for each chart. That would definitely help with accessibility!

@knsv
Copy link
Collaborator

knsv commented Apr 7, 2022

We are all for better accessibility! Much appreciated!

@knsv knsv merged commit f2e7791 into mermaid-js:develop Apr 7, 2022
@gwincr11 gwincr11 mentioned this pull request Apr 7, 2022
9 tasks
@knsv knsv mentioned this pull request May 6, 2022
3 tasks
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.

3 participants