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

Fix the width of comments section on the submissions page. #3070

Conversation

nisusam
Copy link

@nisusam nisusam commented Aug 20, 2016

Fixes: #3057

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.613% when pulling 01970bc on nisusam:3057_horizontal_split_comments_section into b778f4f on exercism:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.613% when pulling 360ae65 on nisusam:3057_horizontal_split_comments_section into b778f4f on exercism:master.

@kytrinyx
Copy link
Member

Would you please include a screenshot of the before/after?

@nisusam
Copy link
Author

nisusam commented Aug 20, 2016

@kytrinyx please see the following screenshots for #3057

Screenshots Before Fix

  • Without Comments

submission_with_horizontal_split_mode_issue without_comment

  • With Comments

submission_with_horizontal_split_mode_issue with_comments

Screenshots After Fix

  • Without Comments

submission_without_comment resolved

  • With Comments

submission_with_comments resolved

@kotp
Copy link
Member

kotp commented Aug 22, 2016

@nisusam it looks like some conflicts happen, can you rebase from master and resolve these?

@nisusam nisusam force-pushed the 3057_horizontal_split_comments_section branch from 360ae65 to fdf7ee0 Compare August 22, 2016 13:38
@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.667% when pulling fdf7ee0 on nisusam:3057_horizontal_split_comments_section into 8ccf8fd on exercism:master.

@Insti
Copy link

Insti commented Aug 22, 2016

Are the Gem updates a necessary part of this patch?

@nisusam
Copy link
Author

nisusam commented Aug 22, 2016

@kotp @Insti fa5be96 has updated application.css, which has used updated version of bootstrap-sass and font-awesome-sass.

I locked the versions of both in Gemfile to avoid the conflicts in future.

@bradhvr
Copy link

bradhvr commented Aug 22, 2016

Hi @nisusam, just so I understand is there something I should have done differently? I'm kind of a novice with some of this. I recall having to install bootstrap-sassand font-awesome-sass separately when trying to set up my development environment.

This is from my Gemfile.lock:

bootstrap-sass (3.3.6)
font-awesome-sass (4.5.0)

@nisusam
Copy link
Author

nisusam commented Aug 22, 2016

@bradhvr Since the version in Gemfile was not specified, when you ran bundle install bundler fetched the latest version for you, which in turn updated the compiled css.

@bradhvr
Copy link

bradhvr commented Aug 22, 2016

Sorry, I'm still a little confused. Wouldn't the change to the compiled css be there because I made a change to application.scss? Compass updated application.css with my change and added the .highlight class.

#3072 also contains some changes to various compiled css based on my modifications.

@tejasbubane
Copy link
Member

@bradhvr Agreed. The application.css should reflect your css changes, but this file in your commit has bootstrap version upgraded to 3.3.7 and fontawesome to 4.6.2.

This is a little odd since the version in Gemfile.lock is 3.3.6 for bootstrap and 4.5.0 for font-awesome.

@tejasbubane
Copy link
Member

My initial guess is not running compass with bundle exec picked up the latest version of these gems installed on your machine, but I may be wrong here.

@Insti
Copy link

Insti commented Aug 22, 2016

If the gemfile modifications are relevant, that's fine but I'd prefer to see them in a separately documented PR rather than bundled with the PR for a minor css change.

@bradhvr
Copy link

bradhvr commented Aug 22, 2016

Oh ok, thanks for the explanation @tejasbubane! The CONTRIBUTING.md file doesn't say to run compass with bundle exec. Should it?

@tejasbubane
Copy link
Member

@bradhvr I am not exactly sure if that is the cause. bootstrap and font-awesome are being required here and imported here.

PS: This would no longer be an issue once #2949 is merged.

@Insti
Copy link

Insti commented Aug 22, 2016

The CONTRIBUTING.md file doesn't say to run compass with bundle exec. Should it?

Yes.

@Insti
Copy link

Insti commented Aug 22, 2016

@nisusam can you untangle the Gem updates from the comments width changes and make a new PR for them please.

@tejasbubane
Copy link
Member

Added bundle exec for compass in #3075

@nisusam nisusam force-pushed the 3057_horizontal_split_comments_section branch from fdf7ee0 to e39e16d Compare August 23, 2016 14:29
@nisusam
Copy link
Author

nisusam commented Aug 23, 2016

@Insti Removed the Gemfile changes. I'll send them in a separate PR after these changes merged.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.667% when pulling e39e16d on nisusam:3057_horizontal_split_comments_section into 8ccf8fd on exercism:master.

kytrinyx pushed a commit that referenced this pull request Aug 24, 2016
@kytrinyx
Copy link
Member

I've merged this locally in 3d26bbf

@kytrinyx kytrinyx closed this Aug 24, 2016
@nisusam nisusam deleted the 3057_horizontal_split_comments_section branch September 29, 2021 13:55
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.

None yet

7 participants