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

Update pagination styles to match Publisher et al [57569170] #22

Merged
merged 6 commits into from Sep 27, 2013

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Sep 26, 2013

  • Change the way bootstrap styles are imported
  • Create an overrides stylesheet
  • Update the border-radius mixin
  • Add a border radius to match the existing table styles
@ghost ghost assigned jamiecobbett and rgarner Sep 26, 2013
@rgarner
rgarner reviewed Sep 26, 2013
View changes
app/assets/stylesheets/bootstrap_and_overrides.scss Outdated
padding: 0 14px;
line-height: 34px;
text-decoration: none;
border: 1px solid #ddd;

This comment has been minimized.

@rgarner

rgarner Sep 26, 2013
Contributor

There are a few of these random colours dotted around. We were using swatch-style imports of, say, status_colours.scss in transition-old. There are a few Bootstrap swatch colours kicking around. Up to you, of course, but it'd be nice either to keep them in one place (like a swatch file) if they're our own or refer to colours defined in bootstrap/_variables otherwise.

This comment has been minimized.

@fofr

fofr Sep 26, 2013
Author Contributor

I agree, I'll update.

@jamiecobbett
Copy link
Contributor

@jamiecobbett jamiecobbett commented Sep 26, 2013

Would you mind changing the commit messages to comply with the styleguide? Specifically, the first line should have a max of 50 chars: https://github.com/alphagov/styleguides/blob/master/git.md#structure

Paul Hayes and others added 4 commits Sep 26, 2013
* Move bootstrap import out of application.scss and into a separate
bootstrap and overrides file
* Add the pagination styles to this new file
* Update application to include bootstrap first, with overrides second
* Add a 4px border radius to match table styles
* Fallthrough to standards compliant unprefixed version
* Override not a tweak
* .active a isn't used
@@ -0,0 +1,43 @@
@import "bootstrap";

This comment has been minimized.

@rgarner

rgarner Sep 26, 2013
Contributor

Can we leave the bootstrap @imports in application.scss? The purpose of that file is pretty much only imports, and I feel we've mixed some unrelated concerns here. And could we call this file pagination.css.scss? I'm not certain they're overrides. Although bootstrap does have a _pagination.scss, the pagination markup generated in this app is from Kaminari. Some of the classes overlap, but I think that's largely coincidental (and if I have to come and fix a pagination styling problem later, I wouldn't think to look in a file with this name).

This comment has been minimized.

@fofr

fofr Sep 27, 2013
Author Contributor

It looks like the bootstrap overrides is a red herring I picked up from the Publisher repo. Good catch, and thanks for pointing out Kaminari. I'll put pagination in its own stylesheet as suggested.

@@ -8,10 +8,8 @@
* You're free to add application-wide styles to this file and they'll appear at the top of the
* compiled file, but it's generally better to create a new file per style scope.
*
*= require bootstrap_and_overrides

This comment has been minimized.

@rgarner

rgarner Sep 26, 2013
Contributor

This line's not necessary - require_tree . will pick it up.

Paul Hayes added 2 commits Sep 27, 2013
* Markup is generated by kaminari not bootstrap
* Rename overrides to pagination.css.scss
* Put boostrap imports back into application.scss
* Avoid random colours, use defined ones instead
* Remove reference to overrides
jamiecobbett added a commit that referenced this pull request Sep 27, 2013
Update pagination styles to match Publisher et al [57569170]
@jamiecobbett jamiecobbett merged commit f4cd0b5 into master Sep 27, 2013
@jamiecobbett jamiecobbett deleted the pagination-ui-57569170 branch Sep 27, 2013
@jamiecobbett
Copy link
Contributor

@jamiecobbett jamiecobbett commented Sep 27, 2013

Good work :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.