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

Fix: compile item text (fixes #173) #174

Merged
merged 3 commits into from
Oct 26, 2023
Merged

Fix: compile item text (fixes #173) #174

merged 3 commits into from
Oct 26, 2023

Conversation

kirsty-hames
Copy link
Contributor

Fixes #173

To support the use of a11y_alt_text helper tag.

PR Test

Add a11y_alt_text helper to Matching item text e.g. {{a11y_alt_text '$5bn' 'five billion dollars'}}.

In your course, go to the Matching item and '$5bn' should be displayed.

Using the browser devtools, inspect '$5bn' and the helper should have split out the on screen text and a11y text e.g.
<span aria-hidden="true">$5bn</span><span class="aria-label">five billion dollars</span>

Copy link
Contributor

@joe-allen-89 joe-allen-89 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 @kirsty-hames, should also add compile to matchingDropDown.jsx so we can use it in the drop down lists?

Copy link
Contributor

Choose a reason for hiding this comment

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

👀

@kirsty-hames
Copy link
Contributor Author

Looks good @kirsty-hames, should also add compile to matchingDropDown.jsx so we can use it in the drop down lists?

Thanks for the suggestion @joe-allen-89. If I do the same for matchingDropDown, the text property is also used for the <li text=""> attribute.
I'm not actually sure what the text attribute is for but I imagine we only need the on screen text rendered here?

I've tried using text={a11y.normalize(compile(text))} but you get both the on screen and screen reader text. See screenshot below. Other than creating a new property to override the text attribute, any other ideas?

normalise_compile

@oliverfoster
Copy link
Member

text={text}

v5.0.0 hbs

parent.$inner.html(this.$el.attr('text'));

It's just left over from old code. It should be safe to remove.

legacy code carried over from v5.0.0 hbs not needed
Copy link
Contributor

@joe-allen-89 joe-allen-89 left a comment

Choose a reason for hiding this comment

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

👍

@joe-allen-89 joe-allen-89 merged commit e224c39 into master Oct 26, 2023
@joe-allen-89 joe-allen-89 deleted the issue/173 branch October 26, 2023 14:44
github-actions bot pushed a commit that referenced this pull request Oct 26, 2023
## [7.3.2](v7.3.1...v7.3.2) (2023-10-26)

### Fix

* compile item text (fixes #173) (#174) ([e224c39](e224c39)), closes [#173](#173) [#174](#174)
@github-actions
Copy link

🎉 This PR is included in version 7.3.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

value={_index}
aria-selected={_isHighlighted || null}
selected={_isHighlighted || null}
onClick={onOptionClicked}
>
<div className="dropdown-item__inner js-dropdown-list-item-inner u-no-select" dangerouslySetInnerHTML={{ __html: displayText || text }}>
<div className="dropdown-item__inner js-dropdown-list-item-inner u-no-select" dangerouslySetInnerHTML={{ __html: displayText || compile(text) }}>
Copy link
Member

Choose a reason for hiding this comment

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

compile doesn't exist in this file as it wasn't imported

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

Successfully merging this pull request may close these issues.

Item text doesn't compile a11y_alt_text helper
4 participants