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.

Sign up for GitHub

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

Jump to bottom

[alternative idea] Use application css #38

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

[alternative idea] Use application css #38

wants to merge 8 commits into from

Conversation

paulkoegel
Copy link
Contributor

this is just an idea and a suggestions. i'm totally fine if you stick with your decisions and conventions.

WHY?
i couldn't overcome issues with integrating global styles (see #35) and started reading the source code. came up with this alternative idea, which i find a lot simpler and serves me as my current workaround / think i'll probably stick with my fork.

WHAT

  • don't construct styleguide's CSS from .scss files in app/components
  • move components' CSS files to app/assets (breaks your idea of putting all related files of a component into one folder)
  • simply load application.css in styleguide -> no more hassle loading global stylesheets, and guaranteed to have the same CSS code as on production
  • use safer CSS class names for styleguides sidebar, navigation etc. mv-... -> .mountainview-styleguide-... - someone out there sure has an .mv-sidebar...
  • don't use a CSS reset for the styleguide (see Styleguide CSS has a CSS reset and modifies <body> #37)
  • load application.css after the styleguide's own mountain_view.css, to make everything overridable from there
  • add class to styleguide's <body> to make its style overridable from application.css

Would love to hear your ideas! and as i said, no hard feelings if you decide against this because you prefer your current approach. would be glad if the issues i came across would be fixed, though :)
thanks for building mountain_view!

Paul Kögel added 8 commits March 17, 2016 12:50
Tests: fix warning.
47892ce 
`mountain_view/test/helpers/mountain_view/component_helper_test.rb:10: warning: ambiguous first argument; put parentheses or a space even after `/' operator`

`/` can be a division or the start of a regex.
Tests: fix deprecation warning.
fd14c10 
`warning: Dir.exists? is a deprecated name, use Dir.exist? instead`
Fix circular dependency warning:
435afde 
mountain_view/lib/mountain_view/engine.rb:2: warning: loading in progress, circular require considered harmful - mountain_view/lib/mountain_view.rb
Make Hound CI even happier.
b7298e7
+ .travis.yml: add Ruby 2.3.
9f19f31 
+ README: add information about supported Ruby
versions.
Merge branch 'ruby-versions' into fork
8cb4e63
Don't build styleguide's CSS from component folder but use
645ead1 
application.scss instead.

+ Move component stylesheets to app/assets/stylesheets instead.
+ remove CSS reset to better ensure the styleguide uses the exact same
CSS as the app
+ add class to stylesheet body that can be modified in
application.scss
+ change styleguide element naming convention from `mv-` to
`mountainview-styleguide-` - should be safer and someone might have an
`.mv-sidebar` in their app.
Undo changes that slipped in from other pull requests.
5b4bd4a
@paulkoegel
Copy link
Contributor Author

Here's the sample Rails app I used to build this mountain view fork:
https://github.com/paulkogel/mountain_view_demo_forked

@kitop
Copy link
Collaborator

kitop commented Mar 17, 2016

Hi @paulkogel. Thanks for contributing!

I like the .mv- to .mountainview-styleguide- rename, makes sense at it makes collisions less probable. And same for adding a class name to the body.

I'm not so sure about adding the global application.css to the styleguide though. Some applications may not have that, but have it broken app separately or named differently.
Also, it may conflict with the styleguide styles if some components are re defined at the tag level (similar to what you describe with reset.css)
Maybe it would be nice to make that configurable, in case somebody wants to overwrite the styleguide.

Also, there's always the chance to add your own app/views/layouts/mountain_view.html.erb in your app and override the gem one. That will give you way more flexibility.

Interested in hear what you think.

@kitop kitop mentioned this pull request Jul 16, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@paulkoegel @kitop