Skip to content

feat(microchart-line): improve accessibility - #1781

Open
akashsonune wants to merge 1 commit into
mainfrom
feat/improve-microchart-line-accessibility
Open

feat(microchart-line): improve accessibility#1781
akashsonune wants to merge 1 commit into
mainfrom
feat/improve-microchart-line-accessibility

Conversation

@akashsonune

@akashsonune akashsonune commented Mar 31, 2026

Copy link
Copy Markdown
Member

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request improves the accessibility of the si-microchart-line component by adding a host role of 'img' and providing descriptive aria-label attributes in the example usage. A review comment suggests refining one of the labels to remove a colon, ensuring compliance with the Siemens UX writing guidelines for concise, telegram-style UI text.

Comment thread src/app/examples/si-ncharts/si-micro-charts.html Outdated
@akashsonune
akashsonune force-pushed the feat/improve-microchart-line-accessibility branch 4 times, most recently from eb6183b to b3246c8 Compare March 31, 2026 14:42
@akashsonune
akashsonune marked this pull request as ready for review March 31, 2026 14:43
@akashsonune
akashsonune requested review from a team as code owners March 31, 2026 14:43
@akashsonune
akashsonune force-pushed the feat/improve-microchart-line-accessibility branch 2 times, most recently from b22ba0c to 64e8ac3 Compare March 31, 2026 17:32
@akashsonune
akashsonune force-pushed the feat/improve-microchart-line-accessibility branch from 64e8ac3 to 3df98f2 Compare March 31, 2026 17:53

@spliffone spliffone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am uncertain that automatically applying a role without description is the right way I suggest we quickly discuss it in core meeting - so everyone is aligned.

Comment thread src/app/examples/si-ncharts/si-micro-charts.html
@akashsonune

akashsonune commented Apr 2, 2026

Copy link
Copy Markdown
Member Author

I am uncertain that automatically applying a role without description is the right way I suggest we quickly discuss it in core meeting - so everyone is aligned.

@spike-rabbit WDYT? I agree with rayk that the role should be supported with a meaningful desc. but whether we should make user specify this or should be a required input (breaking change) in our components, is something we should decide

@spike-rabbit

Copy link
Copy Markdown
Member

It depends if we expect the micro line chart to have relevant information that needs to accessible or just a visual enhancement, that could be omitted.

@panch1739 WDYT?

@akashsonune

Copy link
Copy Markdown
Member Author

It depends if we expect the micro line chart to have relevant information that needs to accessible or just a visual enhancement, that could be omitted

@panch1739 WDYT?

@panch1739 Can you share your input?

@spike-rabbit spike-rabbit added this to the 51.x milestone Jul 8, 2026

@spike-rabbit spike-rabbit left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

After discussion: setting the role img would make an aria-label or similar effectively required. While in most cases this the line chart should be probably exposed to a11y technology, there could be cases where this is not needed.
It also pointless to expose the chart, of the label does not provide a value so actually describing what can be seen.
Enforcing it would potentially lead to a lot of useless labels, so we are better of by not enforcing it at all.

@akashsonune instead of enforcing it in the code, we should actually have a comment about this in the docs.

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