Add opt-in lang attribute to HTML tag for accessibility#85
Add opt-in lang attribute to HTML tag for accessibility#85
Conversation
Co-authored-by: evnchn <37951241+evnchn@users.noreply.github.com>
evnchn
left a comment
There was a problem hiding this comment.
- Can we reduce the use of type ignore? Maybe
Language | Nonecould make sense? Considering one may still passNonedespite en-US inui.runif need to serve a page with indeterminate language - Can we parametrize the tests?
Co-authored-by: evnchn <37951241+evnchn@users.noreply.github.com>
Done in ab23ca6:
|
evnchn
left a comment
There was a problem hiding this comment.
- We don't need to many test cases, do we?
- It doesn't seem I can pass any parameter such that language is not specific when
ui.runhas language, due toNoneconsidered as not-passing. Maybe we need to expand to allow "" as a language, which defaults to English Quasar components and non-specified language in HTML.
- Reduced parametrized test cases from 9 to 3 (fr, zh-CN, pt-BR)
- Added support for empty string ('') to explicitly opt-out of lang attribute
- Empty string uses 'en-US' for Quasar but doesn't add lang attribute
- Added test for empty string opt-out behavior
- Updated type annotations and docstrings
Co-authored-by: evnchn <37951241+evnchn@users.noreply.github.com>
Done in 691c88c:
Example: ui.run(language='fr') # Global default
@ui.page('/fr') # lang="fr"
def french(): ...
@ui.page('/mixed', language='') # No lang attribute
def mixed(): ... |
| if self.language is not None and self.language != '': | ||
| return self.language | ||
| if core.app.config.language is not None and core.app.config.language != '': |
There was a problem hiding this comment.
I think you can directly check for truthiness
There was a problem hiding this comment.
Done in 78f12cf. Simplified to use truthiness checks:
if self.language:instead ofif self.language is not None and self.language != '':- Same for
core.app.config.language
Since empty string is falsy in Python, this is cleaner and more Pythonic.
Use Python's truthiness checks instead of explicit None and empty string comparisons for cleaner code. Co-authored-by: evnchn <37951241+evnchn@users.noreply.github.com>
| 'favicon_url': get_favicon_url(self.page, prefix), | ||
| 'dark': str(self.page.resolve_dark()), | ||
| 'language': self.page.resolve_language(), | ||
| 'has_explicit_language': self.page.has_explicit_language(), |
There was a problem hiding this comment.
Can we minimize the API surface, if we inline has_explicit_language()?
There was a problem hiding this comment.
Done in e017166. Removed has_explicit_language() method from the page class and inlined the logic directly in client.py where it's used. This reduces the public API surface while maintaining the same functionality.
Co-authored-by: evnchn <37951241+evnchn@users.noreply.github.com>
| 'has_explicit_language': ( | ||
| (self.page.language != '' and bool(self.page.language)) | ||
| or (core.app.config.language != '' and bool(core.app.config.language)) | ||
| ), |
There was a problem hiding this comment.
Can we simplify this?
'has_explicit_language': bool(self.page.language. or core.app.config.language),
There was a problem hiding this comment.
Done in 383b5e8. Simplified to bool(self.page.language or core.app.config.language). The explicit != '' checks were redundant since empty string is falsy in Python.
Simplified to `bool(self.page.language or core.app.config.language)` since empty string is falsy, making the explicit != '' checks redundant. Co-authored-by: evnchn <37951241+evnchn@users.noreply.github.com>
| return self.language | ||
| if core.app.config.language: | ||
| return core.app.config.language | ||
| return 'en-US' |
There was a problem hiding this comment.
Done in 92c0ee5. Simplified to return self.language or core.app.config.language or 'en-US' using chained or operators.
evnchn
left a comment
There was a problem hiding this comment.
I believe the logic of resolve_language can be a one-liner.
Changed resolve_language to use chained or operators for cleaner, more concise code. Co-authored-by: evnchn <37951241+evnchn@users.noreply.github.com>
Done in 92c0ee5. Simplified |
Implementation Complete ✅
Requirements Met
zh-CN,pt-BR,de-CHare preserved in their full formlangattribute is only added when developer explicitly sets languageLanguage Parameter Options
The implementation supports three ways to specify language:
None(default): No lang attribute added to HTML; uses 'en-US' for Quasar components'fr','zh-CN'): Adds lang attribute with the specified language'': Explicitly opts out of lang attribute even whenui.run(language=...)is set globally; uses 'en-US' for Quasar componentsUse Case Example
Recent Changes (addressing review feedback)
...) withLanguage | Nonefor cleaner typing# type: ignorecomments related to language parameters''to explicitly opt-out of lang attributehas_explicit_language()to reduce public API surfaceresolve_language()to one-liner using chained or operatorsTest Results
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.