moneyadviceservice / frontend Goto Github PK

#881

@alexwllms https://www.moneyadviceservice.org.uk/en/search?query=pensions

The following pages are inaccessible due to an Internal Server Error:

• https://www.moneyadviceservice.org.uk/en/styleguide/components/common
• https://www.moneyadviceservice.org.uk/en/styleguide/components/website
• https://www.moneyadviceservice.org.uk/en/styleguide/layouts
• https://www.moneyadviceservice.org.uk/en/styleguide/css/typography

See discussion in #834

If this project is open source you may wish to not publicise the secrets.yml config file when we want to actually deploy this project.

https://github.com/moneyadviceservice/frontend/blob/master/config/secrets.yml

Otherwise for example it is possible to modify the cookie contents without the server knowing it has been tampered with.

@andrewtennison
In mas_collapsible.js, there's a keypress event for the spacebar which triggers a 'click' event on the trigger element. This causes the menu to open and then close immediately.

Commenting out this block fixes this issue...

// Accessibility support for spacebar
this.sections[i].trigger.on('keypress', function(e){
  if(e.which === 32) {
    _this.sections[i].trigger.trigger('click');
  }
});

I've tested it in IE9/10/11, Safari, Firefox and Chrome and both spacebar and enter keypresses toggle the menu as expected.

Happy to do a pull request, just wanted to check the reasoning behind it in case I've missed something.

We've had a report back from the DAC on the Savings Calculator. They included the following section regarding the feedback form, which I include below:

The icon to close the questionnaire is very small. This means that someone with mobility issues would not be able to select it due to the very small actionable area.

Happy to make the changes if anyone can point me to the right area. Equally happy for someone else to do it 😜

Karma is causing intermittent failures on Go:

./node_modules/karma/bin/karma start /var/lib/go-agent/pipelines/responsive_commit/frontend/tmp/karma_unit.js20141212-9196-dzbxll --single-run

module.js:340
    throw err;
          ^
Error: Cannot find module './lib/parse-js'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:280:25)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at Object.<anonymous> (/var/lib/go-agent/pipelines/responsive_commit/frontend/node_modules/karma/node_modules/socket.io/node_modules/socket.io-client/node_modules/uglify-js/uglify-js.js:14:17)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.require (module.js:364:17)

To easily mount a tool locally we need a features.yml.sample.

IE issues + possible refactor
child selectors
fontSize mixin
inlineBlock mixin
focus state for IE

Looks labelled to me, but maybe not in all browsers? This is the DAC report:

The search field present is unlabelled, this will cause confusion when screen reader users want to use the search facility. This is consistent across all browsers.
URL: https://qa.test.moneyadviceservice.org.uk/en/tools/mortgage-calculator

The link for Find out more about how you’re protected if you pay by credit card. links to https://cms.moneyadviceservice.org.uk/en/articles/how-youre-protected-when-you-pay-by-card when it should instead link to https://www.moneyadviceservice.org.uk/en/articles/how-youre-protected-when-you-pay-by-card

We have turned off unused classes in the HTML inspector - however we would prefer to ignore certain pattern matches (such as js- wf- etc.).

From DAC:

The chevrons within the breadcrumb trail are labelled as ‘greater than’ and are being picked up as links and make no sense to VoiceOver and TalkBack users.

Relevant code

Flagging this so we don't forget to keep an eye on it.

See: 685b87c & 928a13b

We disabled these because PhantomJS kept crashing when it hit them, which caused the build to fail. This only happens on some machines.

Should be checked again on new PhantomJS releases.

Options:

1. through css (do nothing)
2. as base 64 encoded string in css
3. 64bit string local storage
   • guardian/frontend#29
   • https://github.com/guardian/frontend/blob/master/common/app/assets/javascripts/modules/ui/fonts.js
   • public, but not written in an easy to reuse way. Possible later optimisation
4. google font loader
   • https://github.com/typekit/webfontloader
   • additional JS = 6.57k
   • async, means always get flash
   • inline, blocks first time
     could we extend to load async, set cookie, if cookie set / file cached add class / change loading method?
5. custom JS solution
   • smaller than type kit as less services supported and unused features not in

I have implemented 4 inline but we should review this at a later stage

Problem
Currently Dough contains a number of MAS-specific styles which cannot be overridden directly/easily using the dough_theme layer in frontend.

Solution
I've started a refactor to tidy up the theming layer across frontend and Dough by removing any MAS-related theming and setting up a simple base for Dough. The dough_theme is moved from frontend into its own repo: mas_dough_theme. This allows for the theme to be overridden at an app and engine level.

Frontend now relies on three different dependencies:

1. Dough - provides base CSS styling for Dough components.
2. Yeast - provides grid, mixins, placeholders and functions support for mas_dough_theme and other MAS site components.
3. mas_dough_theme - overrides the default Dough theme with MAS styling.

Progress
The majority of the work is done, the last remaining part is to implement mas_dough_theme @imports across the engines and do some regression testing. @Guntrisoft suggested diffing the generated CSS to check for any abnormalities.

Here's a little CLI command that will diff served files (The left-hand CSS is master and right-hand CSS is the new changes):

sdiff <(curl -s http://localhost:3001/assets/enhanced_responsive.css\?body\=1) <(curl -s http://localhost:5000/assets/enhanced_responsive.css\?body\=1) |grep "<" > diffs.txt 

PRs

• moneyadviceservice/dough#227
• moneyadviceservice/yeast#5
• #1023

An example of the new changes implemented in an engine:

• moneyadviceservice/pensions_calculator#172

@moneyadviceservice/frontend

On the issue #834 we discussed about the concern spec that it was tested with stubs and testing a private method. Due to the freeze it's better we merge the PR but continuing the discussion here.

https://github.com/moneyadviceservice/frontend/blob/master/spec/controllers/concerns/authentication_controller_concern_spec.rb