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

Updates from bedrock #404

Merged
merged 3 commits into from
May 29, 2024
Merged

Updates from bedrock #404

merged 3 commits into from
May 29, 2024

Conversation

MozmarRobot
Copy link
Collaborator

From file changes in mozilla/bedrock@5f44a96

@MozmarRobot MozmarRobot requested a review from a team as a code owner May 24, 2024 09:06
Copy link
Contributor

@peiying2 peiying2 left a comment

Choose a reason for hiding this comment

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

@reemhamz minor issues. Otherwise, it looks good.


home-mozilla-takes-bets = “{ -brand-name-mozilla } is taking bets to show the world there’s a business to be made with trustworthy AI. That includes putting things like human rights, data protection and transparency at the core of how these complex systems work.”

# Politico is a brand name for a news organization
home-politico-cite = Politico

home-join-us-in-shaping = Join us in shaping trustworthy AI
home-work-on-ai = { -brand-name-mozilla }’s work with AI isn’t just a new thing—we’ve spent years funding, building and advocating for AI that’s open, fair and developed responsibly. Our focus is on creating AI that serves the people, prioritizes transparency and supports the public good, not corporate agendas.
Copy link
Contributor

Choose a reason for hiding this comment

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

Use M dash.

Copy link
Contributor

Choose a reason for hiding this comment

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

This:

"work with AI isn’t just a new thing—we’ve spent years funding"

already is U+2014 EM DASH

@peiying2 Do you mean another occurence?

Copy link
Contributor

Choose a reason for hiding this comment

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

The only inconsistent thing here is that while this sentence uses the dash correctly — without the spaces on both sides (at it should be IIRC), the other string on L64 has spaces around it. So the only thing I see here wrt dashes would be making these two occurrences more consistent: mozilla/bedrock#14602 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

OTOH the "correct" (non)spacing produces some ugly line breaks at times:

Screenshot 2024-05-25 at 20 24 02

so the question is whether to give up and just have spaces around it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've asked the copy writing team for clarification on spaces and em dashes. It seems we are doing it a couple different ways currently and since we are trying to go a little more colloquial with our tone, that might be true of our grammar too so I don't feel confident making a call myself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, it was hard to see it. I agree with @janbrasna that there should be a space before and after the M dash, but it has been inconsistent throughout. If there is a branding or style guide somewhere, let me know. For now, I will take this as is and move forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

I talked to someone in brand and they said spaces around the dash are the way to go.

They also said they don't have a style guide.

@@ -57,3 +83,9 @@ home-featured-product = Featured product

# HTML for visual formatting. "Blur" here is used as a metaphor for hiding or obscuring something.
home-feature-blur-your-location = Blur your location & activity using <span>{ -brand-name-mozilla-vpn }</span>

Copy link
Contributor

Choose a reason for hiding this comment

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

remove an extra line break

janbrasna

This comment was marked as resolved.

Comment on lines 84 to 86
navigation-v2-mozilla-ai = { -brand-name-mozilla-ai }

navigation-v2-mozilla-ventures = { -brand-name-mozilla-ventures }
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether these two should be removed for now as not to surprise anyone — the menu items were not actually added in the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

@stephaniehobson who can make this decision? Should we keep them and they will show up eventually?

Choose a reason for hiding this comment

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

For now, it doesn't seem like they're going to be added to the nav anytime soon, so I'm going to remove them from our codebase for now. Thanks for the call-out @janbrasna !

@peiying2 peiying2 merged commit 66c68df into master May 29, 2024
2 checks passed
@peiying2 peiying2 deleted the update-from-bedrock-5f44a967 branch May 29, 2024 15:52
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.

5 participants