Update Notice Module and Module CSS loading API #880

Merged
merged 9 commits into from Aug 28, 2015

Conversation

Projects
None yet
2 participants
@kabel
Contributor

kabel commented Aug 27, 2015

  • Reverts bad commit to debug html instance
  • Changes the example markup for notice
  • Updates the core band styles to use margin to offset the page-pad on mobile
  • Updates notice styles to not do so many overrides
  • Cleanup WDN api to remove dead code and add a method for getting a versioned template file URL
  • Update all modules to use the versioned template file URL getter for loading CSS.

kabel added some commits Aug 24, 2015

Change maincontent forced padding to margin
This will provide better support for legacy/unbanded content that has
existing padding. This may cause a backward compatibility issue with
legacy grids as direct children of maincontent.

Also refactors media queries band styles directly into the selectors
they apply to.
Add custom Modernizr feature test for stylesheet load events
Webkit and Gecko have implemented working event implementations for the
<link> element. This can be used in our loadCSS function.
Improvements to notices plugin markup and styles
Implemented HTML5 data attributes for setting the overlay and auto-close
period. Updated the example HTML to match. The code remains fully
backwards compatible with the css class based initialization.
@mfairchild365

View changes

wdn/templates_4.0/less/layouts/bands.less
@media @bp3 {
- padding: 0;
+ padding-left: 0;
+ paading-right: 0;

This comment has been minimized.

This comment has been minimized.

@kabel

kabel Aug 27, 2015

Contributor

Indeed.

@kabel

kabel Aug 27, 2015

Contributor

Indeed.

</div>
</div>
+<script type="text/javascript">
+WDN.initializePlugin('notice');
+</script>
</div>
</body>
</html>

This comment has been minimized.

@mfairchild365

mfairchild365 Aug 27, 2015

Member

I wonder if these examples could use some accessibility love. Perhaps aria roles of alert and/or alertdialog. I also wonder if it is possible to have the JavaScript side of things add the appropriate role automatically, so it doesn't have to be hand coded.

@mfairchild365

mfairchild365 Aug 27, 2015

Member

I wonder if these examples could use some accessibility love. Perhaps aria roles of alert and/or alertdialog. I also wonder if it is possible to have the JavaScript side of things add the appropriate role automatically, so it doesn't have to be hand coded.

This comment has been minimized.

@kabel

kabel Aug 27, 2015

Contributor

They could, but this is beyond the scope of this PR.

@kabel

kabel Aug 27, 2015

Contributor

They could, but this is beyond the scope of this PR.

This comment has been minimized.

@mfairchild365

mfairchild365 Aug 27, 2015

Member

Is it beyond the scope? You are already refactoring it, accessibility should be in mind.

I am curious exactly how AT processes these roles if they exist on the page on initial page load. Do they announce them right away? I will have to check.

@mfairchild365

mfairchild365 Aug 27, 2015

Member

Is it beyond the scope? You are already refactoring it, accessibility should be in mind.

I am curious exactly how AT processes these roles if they exist on the page on initial page load. Do they announce them right away? I will have to check.

This comment has been minimized.

@kabel

kabel Aug 27, 2015

Contributor

That's like saying, "hey, I see you changing something in this file, mind changing something else mostly unrelated to your original intent?"

Make your own PR. :P

@kabel

kabel Aug 27, 2015

Contributor

That's like saying, "hey, I see you changing something in this file, mind changing something else mostly unrelated to your original intent?"

Make your own PR. :P

This comment has been minimized.

@mfairchild365

mfairchild365 Aug 27, 2015

Member

Sorry, I don't see how accessibility is mostly unrelated. The example code was updated to include the latest best practices, which should include accessibility considerations.

@mfairchild365

mfairchild365 Aug 27, 2015

Member

Sorry, I don't see how accessibility is mostly unrelated. The example code was updated to include the latest best practices, which should include accessibility considerations.

This comment has been minimized.

@kabel

kabel Aug 27, 2015

Contributor

I'm not trying to be confrontational, but this is exactly what feature creep is. If you see something else that needs improvement, you should create your own pull request.

@kabel

kabel Aug 27, 2015

Contributor

I'm not trying to be confrontational, but this is exactly what feature creep is. If you see something else that needs improvement, you should create your own pull request.

This comment has been minimized.

@mfairchild365

mfairchild365 Aug 27, 2015

Member

I'm not trying to be confrontational either. I'd argue that feature creep is not this. Pull requests should be reviewed for code style, bugs, usability and risk. Risk and usability includes accessibility. If care was taken to refactor something to make it better (and this is much better than it was before), accessibility should be reviewed in the pull request.

Feature creep on the other hand is adding additional superficial (maybe the wrong word) features, such as different fading transitions, or different colors because they would look nice, or a different widget entirely.

When a widget is updated, all aspects of that widget in terms of code and usability should be visited and addressed.

Perhaps that is just my professional opinion.

@mfairchild365

mfairchild365 Aug 27, 2015

Member

I'm not trying to be confrontational either. I'd argue that feature creep is not this. Pull requests should be reviewed for code style, bugs, usability and risk. Risk and usability includes accessibility. If care was taken to refactor something to make it better (and this is much better than it was before), accessibility should be reviewed in the pull request.

Feature creep on the other hand is adding additional superficial (maybe the wrong word) features, such as different fading transitions, or different colors because they would look nice, or a different widget entirely.

When a widget is updated, all aspects of that widget in terms of code and usability should be visited and addressed.

Perhaps that is just my professional opinion.

@mfairchild365

This comment has been minimized.

Show comment
Hide comment
@mfairchild365

mfairchild365 Aug 28, 2015

Member

@kabel it looks like the tests are failing for this one.

Member

mfairchild365 commented Aug 28, 2015

@kabel it looks like the tests are failing for this one.

@kabel

This comment has been minimized.

Show comment
Hide comment
@kabel

kabel Aug 28, 2015

Contributor

Boo. It seems like a side-effect. I'm restarting the build to confirm.

Contributor

kabel commented Aug 28, 2015

Boo. It seems like a side-effect. I'm restarting the build to confirm.

@mfairchild365

This comment has been minimized.

Show comment
Hide comment
@mfairchild365

mfairchild365 Aug 28, 2015

Member

Looks like it is still failing. :(

Member

mfairchild365 commented Aug 28, 2015

Looks like it is still failing. :(

@kabel

This comment has been minimized.

Show comment
Hide comment
@kabel

kabel Aug 28, 2015

Contributor

Found it.

Contributor

kabel commented Aug 28, 2015

Found it.

mfairchild365 added a commit that referenced this pull request Aug 28, 2015

Merge pull request #880 from kabel/notice-cleanup
Update Notice Module and Module CSS loading API

@mfairchild365 mfairchild365 merged commit 81aa4cb into unl:develop Aug 28, 2015

1 check passed

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

@kabel kabel deleted the kabel:notice-cleanup branch Aug 28, 2015

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