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 bug 1524602 - Hide Editor actions and settings when user is logged out. #1205

Merged
merged 3 commits into from
Feb 19, 2019

Conversation

adngdb
Copy link
Collaborator

@adngdb adngdb commented Feb 18, 2019

No description provided.

@adngdb adngdb requested a review from mathjazz February 18, 2019 13:57
@adngdb
Copy link
Collaborator Author

adngdb commented Feb 18, 2019

So, this doesn't work yet, because the text Sign in to translate doesn't have the link set correctly.

In the DOM I see this:

<p class="banner">
    <a href="/accounts/fxa/login/?scope=profile%3Auid+profile%3Aemail+profile%3Adisplay_name"></a>
    Sign in to translate.
</p>

I'm at a complete loss there... @stasm, please, help.

@adngdb
Copy link
Collaborator Author

adngdb commented Feb 18, 2019

It's solved thanks for @stasm! <link> was not usable in a Fluent template because it is a reserved HTML tag, as far as I understand. Using <a> instead is fine.

@stasm
Copy link
Contributor

stasm commented Feb 18, 2019

It's reserved and it's not allowed to have any content. It's content model is defined to be empty. So <link>Foo</link> parses as [<link>, "Foo"], and the closing </link> is ignored.

Copy link
Collaborator

@mathjazz mathjazz left a comment

Choose a reason for hiding this comment

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

Fix tests and we're good! Well done!

editor-editor-button-copy = Copy
editor-editor-button-clear = Clear
editor-editor-button-save = Save
editor-editor-button-suggest = Suggest
Copy link
Collaborator

Choose a reason for hiding this comment

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

editor-editor looks weird... Is that because you're simply following the $module-$component convention?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I don't like it much either. At some point we should have a conversation about those IDs, and decide on a clear rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1528926 to kick off the conversation about the id naming scheme.

<Localized
id="editor-editor-sign-in-to-translate"
a={
<a href='/accounts/fxa/login/?scope=profile%3Auid+profile%3Aemail+profile%3Adisplay_name'></a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is breaking tests:

./src/modules/editor/components/Editor.js
  Line 86:  Anchors must have content and the content must be accessible by a screen reader  jsx-a11y/anchor-has-content

@adngdb adngdb merged commit e05ee3a into mozilla:master Feb 19, 2019
@adngdb adngdb deleted the 1524602-hide-actions-logged-out branch February 19, 2019 14:30
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