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

Sample skin + docs #235

Merged
merged 4 commits into from
Oct 7, 2016
Merged

Sample skin + docs #235

merged 4 commits into from
Oct 7, 2016

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Oct 6, 2016

This adds a skin-sample folder, containing a simple sample skin, and a README describing how the skinning works, where to look, and what to override.

If everybody agrees with the wording of the README, I'll do a similar PR with just the readme to ManageIQ/guides so that it's part of the devel docs as well.

Applying the skin: skin-sample/link.sh (optionally with the path to the ssui folder)
Unapplying: skin-sample/unlink.sh (or rm client/skin)

Closes #115

@himdel
Copy link
Contributor Author

himdel commented Oct 6, 2016

@epwinchell if you ever have some extra time, maybe this sample skin deserves some love so that it actually looks good. But it is just a sample skin, so..

@miq-bot
Copy link
Member

miq-bot commented Oct 6, 2016

Checked commits https://github.com/himdel/manageiq-ui-self_service/compare/07808e3731d19543093f6f90e4f96b5b4ba16f28~...69a4d1c7f502e2491a3325730a3021fa66713385 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
0 files checked, 0 offenses detected
Everything looks good. 👍

@himdel
Copy link
Contributor Author

himdel commented Oct 6, 2016

The CC issues can't be fixed IMO...

BTW @AllenBW should the scss linter check css files too? At least 2 of the errors don't even make sense for css..

@AllenBW
Copy link
Member

AllenBW commented Oct 6, 2016

@himdel I would say yessssssss we should be checking css files ALL TEH FILES

seriously though, if these things cant be changed, welp tough luck codeclimate we're not changing them!

<3

Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤘 ❤️

Copy link
Contributor

@chriskacerguis chriskacerguis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please address the code climate issues?

@chriskacerguis chriskacerguis self-assigned this Oct 7, 2016
@himdel
Copy link
Contributor Author

himdel commented Oct 7, 2016

@chriskacerguis Nope, sorry, I cannot :). Not without breaking it..

@chriskacerguis
Copy link
Contributor

@himdel not arguing, but for the sake of completeness could you maybe shed some light on to why you feel that way?

@himdel
Copy link
Contributor Author

himdel commented Oct 7, 2016

Yup, already on it.. :)

Selector splash__message should be written in lowercase with hyphens

Not without breaking everybody's skin.. - it's overriding styling for this class (actaully, we're using BEM AFAIK so .. different convention even - will investigate)

Begin pseudo elements with double colons: ::

Not in CSS

Color literals like #404 should only be used in variable declarations; they should be referred to via variable everywhere else.

Not in CSS.

Similar code found in 1 other location (mass = 111)

Well, yes, it's an override.

@chriskacerguis
Copy link
Contributor

Thanks @himdel makes sense to me!

@chriskacerguis
Copy link
Contributor

Merging due to @himdel code additions being "valid".

@chriskacerguis chriskacerguis added this to the Sprint 48 Ending Oct 24, 2016 milestone Oct 7, 2016
@chriskacerguis chriskacerguis merged commit dc6b34b into ManageIQ:master Oct 7, 2016
@himdel himdel deleted the sample-skin branch October 10, 2016 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants