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

Bug 1370594: Sample output, border and buttons changes #4402

Merged
merged 1 commit into from Sep 8, 2017

Conversation

Projects
None yet
3 participants
@stephaniehobson
Contributor

stephaniehobson commented Sep 2, 2017

  • Remove wiki_samples waffle flag, this feature is now permanent.
  • Lint wiki-samples.js.
  • Capitalize CodePen and JSFiddle names.
  • Convert button-link to a mixin and apply to .open-in buttons.
  • Added top margin to .cta-link mixin
    • Decrease margin on homepage search form to account for that.
  • Add sample_frame waffle
    • Make width 100% by default
    • Make border match code examples by deafult
    • Change buttons to use .cta-link styles as recommended by Brigade ages ago.

Also:

  • Remove outdated summary styles from wysiqyg style sheet.

Testing notes:
I waffled this feature a little differently. I didn't want to duplicate the styles sheets again so I added a class to the body if the sample_frame waffle is enabled. This means we can roll this feature into production independent of the line_length waffle if we want. All the code still had to be duplicated between the two sets of .scss files though. I suggest reviewing the code for the one in /components/ and I'll copy any requested changes to the one in /components-skinny/ as well.

@stephaniehobson stephaniehobson requested a review from schalkneethling Sep 2, 2017

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 2, 2017

Codecov Report

Merging #4402 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4402   +/-   ##
=======================================
  Coverage   94.73%   94.73%           
=======================================
  Files         254      254           
  Lines       22620    22620           
  Branches     1666     1666           
=======================================
  Hits        21428    21428           
  Misses        963      963           
  Partials      229      229

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ab1304...e41cd84. Read the comment docs.

codecov-io commented Sep 2, 2017

Codecov Report

Merging #4402 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4402   +/-   ##
=======================================
  Coverage   94.73%   94.73%           
=======================================
  Files         254      254           
  Lines       22620    22620           
  Branches     1666     1666           
=======================================
  Hits        21428    21428           
  Misses        963      963           
  Partials      229      229

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ab1304...e41cd84. Read the comment docs.

@schalkneethling

This comment has been minimized.

Show comment
Hide comment
@schalkneethling

schalkneethling Sep 4, 2017

Collaborator

This looks really good. But, for some reason I am running into the following issue in Firefox(both release and nightly). On the landing page I get a JS error:

TypeError: mdn.analytics is undefined

and then, on a page such as /docs/Web, I also get:

TypeError: win.mdn.getUrlParameter is not a function

Also, the drop down menu does not work. This happens whether I am logged in or not. This does not happen on production though. Can you replicate this behaviour?

Collaborator

schalkneethling commented Sep 4, 2017

This looks really good. But, for some reason I am running into the following issue in Firefox(both release and nightly). On the landing page I get a JS error:

TypeError: mdn.analytics is undefined

and then, on a page such as /docs/Web, I also get:

TypeError: win.mdn.getUrlParameter is not a function

Also, the drop down menu does not work. This happens whether I am logged in or not. This does not happen on production though. Can you replicate this behaviour?

Bug 1370594: Sample output, border and buttons changes
- Remove wiki_samples waffle flag.
- Lint wiki-samples.js
- Capitalize CodePen and JSFiddle
- Converte button-link to a mixin and apply to .open-in buttons
- Added top margin to .cta-link mixin
    - Decrease margin on homepage search form to account for that
- Add `sample_frame` waffle
    - Make width 100% by default
    - Make border match code examples by deafult
    - Change buttons to use .cta-link styles

Also:
- Remove summary styles from wysiqyg style sheet (not needed there)
@stephaniehobson

This comment has been minimized.

Show comment
Hide comment
@stephaniehobson

stephaniehobson Sep 5, 2017

Contributor

I can't reproduce on this branch even playing around with my GA settings.

There was a bug on master that was fixed by #4394 that might have caused this behaviour.

Can you please rebase on master and tell me if you're still getting the same behaviour?

Contributor

stephaniehobson commented Sep 5, 2017

I can't reproduce on this branch even playing around with my GA settings.

There was a bug on master that was fixed by #4394 that might have caused this behaviour.

Can you please rebase on master and tell me if you're still getting the same behaviour?

@schalkneethling

This comment has been minimized.

Show comment
Hide comment
@schalkneethling

schalkneethling Sep 6, 2017

Collaborator

I can't reproduce on this branch even playing around with my GA settings.

Ok, it is the uBlock extension that is blocking the loading of /js/analytics.js - The reason is very cryptic

screen shot 2017-09-06 at 11 42 25

I am going to see if I can get an answer from the dev but, does not block this PR.

Collaborator

schalkneethling commented Sep 6, 2017

I can't reproduce on this branch even playing around with my GA settings.

Ok, it is the uBlock extension that is blocking the loading of /js/analytics.js - The reason is very cryptic

screen shot 2017-09-06 at 11 42 25

I am going to see if I can get an answer from the dev but, does not block this PR.

@schalkneethling

r+ 🐾

@stephaniehobson

This comment has been minimized.

Show comment
Hide comment
@stephaniehobson

stephaniehobson Sep 6, 2017

Contributor

Ah, and that problem won't happen on prod because the file is bundled into some other stuff.

Contributor

stephaniehobson commented Sep 6, 2017

Ah, and that problem won't happen on prod because the file is bundled into some other stuff.

@stephaniehobson stephaniehobson merged commit 5226e4b into master Sep 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jwhitlock jwhitlock deleted the 1370594-sample-code branch Sep 21, 2017

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