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

Clear dt.attribute-label past left-floated elements #231

Conversation

calumhalcrow
Copy link
Contributor

Fixes #230

@c-lliope
Copy link
Contributor

@calumhalcrow I just merged #188, which adds visual regression testing. I'd like to run all CSS/HTML changes through that test.

Would you mind rebasing on top of master so this can run through those tests? Thanks!

@calumhalcrow calumhalcrow force-pushed the attribute-label-clears-left-floated-elements branch from 50c2143 to 365d607 Compare November 16, 2015 01:00
@calumhalcrow
Copy link
Contributor Author

@Graysonwright I have rebased onto master and it appears that tests pass (although maybe that in itself is a problem since this change can change the UI visibly?).

BTW, as a side-note: when I run tests locally (after following the Contributing guide), I get an error:

$ bundle exec appraisal rake
>> BUNDLE_GEMFILE=/Users/calum/Projects/administrate/gemfiles/sass_3_4.gemfile bundle exec rake
warning: parser/current is loading parser/ruby22, which recognizes
warning: 2.2.3-compliant syntax, but you are running 2.2.0.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
warning: parser/current is loading parser/ruby22, which recognizes
warning: 2.2.3-compliant syntax, but you are running 2.2.0.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
/Users/calum/.rvm/gems/ruby-2.2.0/gems/activesupport-4.2.2/lib/active_support/dependencies.rb:274:in `require': cannot load such file -- percy/capybara/rspec (LoadError)
    from /Users/calum/.rvm/gems/ruby-2.2.0/gems/activesupport-4.2.2/lib/active_support/dependencies.rb:274:in `block in require'
    from /Users/calum/.rvm/gems/ruby-2.2.0/gems/activesupport-4.2.2/lib/active_support/dependencies.rb:240:in `load_dependency'
    from /Users/calum/.rvm/gems/ruby-2.2.0/gems/activesupport-4.2.2/lib/active_support/dependencies.rb:274:in `require'
    from /Users/calum/Projects/administrate/spec/rails_helper.rb:10:in `<top (required)>'
    from /Users/calum/Projects/administrate/spec/administrate/views/fields/has_many/_show_spec.rb:1:in `require'
    from /Users/calum/Projects/administrate/spec/administrate/views/fields/has_many/_show_spec.rb:1:in `<top (required)>'
    from /Users/calum/.rvm/gems/ruby-2.2.0/gems/rspec-core-3.1.7/lib/rspec/core/configuration.rb:1105:in `load'
    from /Users/calum/.rvm/gems/ruby-2.2.0/gems/rspec-core-3.1.7/lib/rspec/core/configuration.rb:1105:in `block in load_spec_files'
    from /Users/calum/.rvm/gems/ruby-2.2.0/gems/rspec-core-3.1.7/lib/rspec/core/configuration.rb:1105:in `each'
    from /Users/calum/.rvm/gems/ruby-2.2.0/gems/rspec-core-3.1.7/lib/rspec/core/configuration.rb:1105:in `load_spec_files'
    from /Users/calum/.rvm/gems/ruby-2.2.0/gems/rspec-core-3.1.7/lib/rspec/core/runner.rb:96:in `setup'
    from /Users/calum/.rvm/gems/ruby-2.2.0/gems/rspec-core-3.1.7/lib/rspec/core/runner.rb:84:in `run'
    from /Users/calum/.rvm/gems/ruby-2.2.0/gems/rspec-core-3.1.7/lib/rspec/core/runner.rb:69:in `run'
    from /Users/calum/.rvm/gems/ruby-2.2.0/gems/rspec-core-3.1.7/lib/rspec/core/runner.rb:37:in `invoke'
    from /Users/calum/.rvm/gems/ruby-2.2.0/gems/rspec-core-3.1.7/exe/rspec:4:in `<main>'
/Users/calum/.rvm/rubies/ruby-2.2.0/bin/ruby -I/Users/calum/.rvm/gems/ruby-2.2.0/gems/rspec-core-3.1.7/lib:/Users/calum/.rvm/gems/ruby-2.2.0/gems/rspec-support-3.1.2/lib /Users/calum/.rvm/gems/ruby-2.2.0/gems/rspec-core-3.1.7/exe/rspec --pattern spec/\*\*\{,/\*/\*\*\}/\*_spec.rb failed

I was able to fix it with this change:

$ git diff
diff --git a/gemfiles/sass_3_4.gemfile b/gemfiles/sass_3_4.gemfile
index b8c658f..622e42b 100644
--- a/gemfiles/sass_3_4.gemfile
+++ b/gemfiles/sass_3_4.gemfile
@@ -37,6 +37,7 @@ group :test do
   gem "shoulda-matchers", "~> 2.8.0", :require => false
   gem "timecop"
   gem "webmock"
+  gem "percy-capybara"
 end

 group :staging, :production do

I'm not sure if this is the appropriate thing to do though. Should I open a PR?

@c-lliope
Copy link
Contributor

@calumhalcrow Looks like that gemfile was fixed in 079c17f - rebasing again should sort it out.

I'm curious why the build on CircleCI doesn't fail for that - it would be much easier to keep our Appraisal gemfiles up-to-date if we had feedback on that.

@calumhalcrow calumhalcrow force-pushed the attribute-label-clears-left-floated-elements branch from 365d607 to d695f85 Compare November 19, 2015 15:13
@calumhalcrow
Copy link
Contributor Author

@Graysonwright I have rebased. Tests pass locally for me, but CircleCI is failing due to what seems a setup issue on their end:

"Your Ruby version is 2.2.0, but your Gemfile specified 2.2.3"

@c-lliope
Copy link
Contributor

@calumhalcrow yeah, I saw that error last night. We're working on a fix in #253.

@c-lliope
Copy link
Contributor

Looks like it's fixed - we just needed to re-run the tests without the cache.

@c-lliope
Copy link
Contributor

This looks good!

It would be great to have a test for this case, but without being able to customize labels for a field it'll be fairly difficult to set up the test case. I'm okay skipping the test for now.

@c-lliope
Copy link
Contributor

Merged in as a847d57. Cheers!

@c-lliope c-lliope closed this Nov 20, 2015
c-lliope added a commit that referenced this pull request Dec 9, 2015
Changes:

```
* [#251] [FEATURE] Raise a helpful error when an attribute is missing from
  `ATTRIBUTE_TYPES`
* [#298] [FEATURE] Support ActiveRecord model I18n translations
* [#312] [FEATURE] Add a `nil` option to `belongs_to` form fields
* [#231] [UI] Fix layout issue on show page where a long label next to an empty
  value would cause following fields on the page to be mis-aligned.
* [#309] [UI] Fix layout issue in datetime pickers where months and years
  would not wrap correctly.
* [#306] [UI] Wrap long text lines (on word breaks) on show pages
* [#214] [UI] Improve header layout when there is a long page title
* [#198] [UI] Improve spacing around bottom link in sidebar
* [#206] [UI] Left-align checkboxes in boolean form fields
* [#315] [UI] Remove the `IDS` suffix for `HasMany` form field labels
* [#259] [BUGFIX] Make installation generator more robust
  by ignoring dynamically generated, unnamed models
* [#243] [BUGFIX] Fix up a "Show" button on the edit page that was not using the
  `display_resource` method.
* [#248] [BUGFIX] Improve polymorphic relationship's dashboard class detection.
* [#247] [BUGFIX] Populate `has_many` and `belongs_to` select boxes
  with the current value of the relationship.
* [#217] [I18n] Dutch
* [#263] [I18n] Swedish
* [#272] [I18n] Danish
* [#270] [I18n] Don't apologize about missing relationship support.
* [#237] [I18n] Fix broken paths for several I18n files (de, es, fr, pt-BR, vi).
* [#266] [OPTIM] Save a few database queries by using cached counts
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants