Skip to content

Replace pebbles basic tempalate with protocol article template#7141

Merged
craigcook merged 9 commits intomozilla:masterfrom
stephaniehobson:protocol-basic
Jul 16, 2019
Merged

Replace pebbles basic tempalate with protocol article template#7141
craigcook merged 9 commits intomozilla:masterfrom
stephaniehobson:protocol-basic

Conversation

@stephaniehobson
Copy link
Copy Markdown
Contributor

@stephaniehobson stephaniehobson commented May 1, 2019

Description

  • Add .mzp-t-mozilla where appropriate
  • Edit and rename base-pebbles-basic to base-article
  • Convert page specific .less files to .scss files
  • Convert page specific .scss files to use Protocol not Pebbles
  • Change classnames from prose to mzp-u-list-styled
  • Change classnames from data-table to mzp-u-data-table
  • Remove unused styles
  • Tweak page styles as needed

Issue / Bugzilla link

#6822

Testing

@craigcook craigcook self-assigned this May 1, 2019
@stephaniehobson stephaniehobson marked this pull request as ready for review June 25, 2019 21:15
@stephaniehobson
Copy link
Copy Markdown
Contributor Author

This is ready for review now.

Note that I have left the firefox-enterprise style sheet in Pebbles because I'm working on the enterprise section of the site next week.

@stephaniehobson stephaniehobson added Needs Review Awaiting code review P3 Third level priority - Nice to have Review: S Code review time: 30 mins to 1 hour labels Jun 25, 2019
@stephaniehobson
Copy link
Copy Markdown
Contributor Author

This is blocking my work updating the Enterprise section. Upping the priority level of the review.

@stephaniehobson stephaniehobson added P1 First level priority - Must have and removed P3 Third level priority - Nice to have labels Jun 27, 2019
Comment thread bedrock/mozorg/templates/mozorg/about/policy/patents/index.html Outdated
Comment thread bedrock/security/templates/security/client-bug-bounty.html Outdated
@stephaniehobson
Copy link
Copy Markdown
Contributor Author

Nits picked ✅

Thanks @pdehaan

@stephaniehobson
Copy link
Copy Markdown
Contributor Author

Upping the size of this review after talking to @craigcook

@stephaniehobson stephaniehobson added Review: M Code review time: 1-2 hours and removed Review: S Code review time: 30 mins to 1 hour labels Jun 28, 2019
Comment thread bedrock/base/templates/macros-protocol.html Outdated
Comment thread bedrock/mozorg/templates/mozorg/about/forums/forums.html Outdated
Copy link
Copy Markdown
Contributor

@alexgibson alexgibson left a comment

Choose a reason for hiding this comment

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

This touches a lot of pages, not all of which I've gone through manually yet - but I took a first pass. Generally things look good, although I noted some potential UI issues. Some things still feel a little rough around the edges in places.

There's also an issue with the newsletter JS throwing errors, and the side menu is missing some JS to make it functional.

Comment thread bedrock/firefox/templates/firefox/enterprise/signup-thanks.html
Comment thread bedrock/firefox/templates/firefox/enterprise/signup.html
Comment thread media/static-bundles.json Outdated
Comment thread bedrock/mozorg/templates/mozorg/about/forums/forums.html Outdated
Comment thread bedrock/base/templates/base-protocol-basic.html Outdated
Comment thread bedrock/base/templates/base-protocol-basic.html Outdated
Comment thread bedrock/mozorg/templates/mozorg/about-base.html Outdated
Comment thread bedrock/mozorg/templates/mozorg/about/forums/forums.html Outdated
@stephaniehobson stephaniehobson added P3 Third level priority - Nice to have and removed P1 First level priority - Must have labels Jul 8, 2019
@stephaniehobson
Copy link
Copy Markdown
Contributor Author

Well darn, I don't have enough information to populate the mobile version of the side menu and the component is not flexible enough to work without it. I'm going to mark this as blocked and remove the review priority while I think about how to handle this.

@stephaniehobson stephaniehobson added Blocked and removed Needs Review Awaiting code review P3 Third level priority - Nice to have labels Jul 8, 2019
@stephaniehobson
Copy link
Copy Markdown
Contributor Author

Also blocked by mozilla/protocol#402 and a new version of Protocol.

@stephaniehobson stephaniehobson changed the title protocol-basic Replace pebbles basic tempalate with protocol article template Jul 12, 2019
@stephaniehobson
Copy link
Copy Markdown
Contributor Author

Unblocked. Turns out we have the word "Menu" in main.lang! And this update now includes Protocol 7.0.2 with half the fix to the newsletter related js error.

Also, I lost a bike shedding argument with myself and changed the name space from "basic" to "article" as I thought was was more descriptive and easier to differentiate from "base".

Still waiting on:

  • make the newsletter image smaller
  • lighten the background colour of side menu on mobile.

@stephaniehobson
Copy link
Copy Markdown
Contributor Author

Okay, ready for re-review.

@craigcook
Copy link
Copy Markdown
Contributor

On some pages the mzp-c-article is overflowing its mzp-l-main container, but I can't understand why when it has no declared width. It seems somehow related to the float though? If I uncheck the float in devtools it behaves normally. It's weird.

basic-float-overflow

@craigcook
Copy link
Copy Markdown
Contributor

craigcook commented Jul 12, 2019

D'oh! It's the pre for the PGP key at the bottom of the page. But of course. Maybe we need to make it overflow:auto so it can scroll if it needs to?

Copy link
Copy Markdown
Contributor

@craigcook craigcook left a comment

Choose a reason for hiding this comment

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

Looks great! I like the mobile menu treatment, and hooray for losing the highlight style headlines. Just some nitpicks about nested semantics and the overflowing pre, neither of which is actually a showstopper. Nice work!

Comment thread bedrock/base/templates/base-article.html
Comment thread bedrock/mozorg/templates/mozorg/about/forums/cancellation.html
@stephaniehobson
Copy link
Copy Markdown
Contributor Author

Updated to remove article markup from security pages. Reviewing that commit with white-space changes hidden will make it much more readable.

Still fighting with the <pre>.

- Add .mzp-t-mozilla where apprpriate
- Edit and rename base-pebbles-basic to base-protocol-basic
- Convert page specific .less files to .scss files
- Convert page specific .scss files to use Protocol not Pebbles
- Change classnames from prose to mzp-u-list-styled
- Change classnames from data-table to mzp-u-data-table
- Remove unused styles
- Tweak page styles as needed
- Protocol 7.0.2
- Change template and assets to use "article" namespace instead of "basic"
- create basic-article.js bundle and include
    - including making side-bar boxes shrink on mobile
@stephaniehobson
Copy link
Copy Markdown
Contributor Author

Ready for re-review!

@stephaniehobson
Copy link
Copy Markdown
Contributor Author

Also filed mozilla/protocol#422 to back-port the fix for the <pre> problem.

@stephaniehobson stephaniehobson added the P2 Second level priority - Should have label Jul 16, 2019
@craigcook craigcook dismissed alexgibson’s stale review July 16, 2019 23:04

changes were addressed

@craigcook craigcook merged commit 1210bf5 into mozilla:master Jul 16, 2019
@stephaniehobson stephaniehobson deleted the protocol-basic branch July 17, 2019 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Second level priority - Should have Review: M Code review time: 1-2 hours

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants