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

enhancement: extract styles to a vue-form-generator.css file. #27

Closed

Conversation

lionel-bijaoui
Copy link
Member

#20

@lionel-bijaoui lionel-bijaoui mentioned this pull request Aug 8, 2016
5 tasks
@coveralls
Copy link

coveralls commented Aug 8, 2016

Coverage Status

Coverage remained the same at 87.245% when pulling 21c8876 on lionel-bijaoui:LB_extract_style into 66d201d on icebob:master.

@icebob
Copy link
Member

icebob commented Aug 8, 2016

Thanks!

Please remove the scoped from styles in field*.vue
https://github.com/icebob/vue-form-generator/blob/master/src/fields/fieldText.vue#L14

@lionel-bijaoui
Copy link
Member Author

lionel-bijaoui commented Aug 8, 2016

I'm all for it, but as is, it will have repercussion on the style. Most css selector are input and span. They will need a class, no ?
Edit: also, it would be cool to have css-nano do some cleanup after (there are duplicate)

@coveralls
Copy link

coveralls commented Aug 8, 2016

Coverage Status

Coverage remained the same at 87.245% when pulling 4a4641c on lionel-bijaoui:LB_extract_style into 66d201d on icebob:master.

@coveralls
Copy link

coveralls commented Aug 8, 2016

Coverage Status

Coverage remained the same at 87.245% when pulling 18cbeef on lionel-bijaoui:LB_extract_style into 66d201d on icebob:master.

@icebob
Copy link
Member

icebob commented Aug 8, 2016

Yes. Need a .vue-form-generator class.

2016.08.08. 16:57 ezt írta ("lionel-bijaoui" [email protected]):

I'm all for it, but as is, it will have repercussion on the style. Most
css selector are input and span. They will need a class, no ?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#27 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAStWXZMW945lmE3tk7pS5_TcC-wP-Mlks5qd0PdgaJpZM4JfHam
.

@lionel-bijaoui
Copy link
Member Author

@icebob I will refactor the CSS in this branch, if it is OK for you. Approved ?

@icebob
Copy link
Member

icebob commented Aug 8, 2016

Ok.

2016.08.08. 17:54 ezt írta ("lionel-bijaoui" [email protected]):

@icebob https://github.com/icebob I will refactor the CSS in this
branch, if it is OK for you. Approved ?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#27 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAStWco0IKtnoqj1JQmIsvwrSbraNLapks5qd1EygaJpZM4JfHam
.

@icebob
Copy link
Member

icebob commented Aug 8, 2016

We add .vue-form-generator class for fieldset in https://github.com/icebob/vue-form-generator/blob/master/src/formGenerator.vue#L2

@lionel-bijaoui
Copy link
Member Author

lionel-bijaoui commented Aug 8, 2016

I'm also going to add .field-*** for component specific class override (e.g. .field-checkbox).
I also noticed that some field components have multiple top-level elements. This will create fragment instance that should be avoided.
Can I fix it too or should I open an issue and fix it later? #28
Having it done now will be easier especially with these class override (I can place this class on the top-level element)

@icebob
Copy link
Member

icebob commented Aug 8, 2016

@lionel-bijaoui please don't add .field-*** classes! They have. Check this line: https://github.com/icebob/vue-form-generator/blob/master/src/formGenerator.vue#L101
So only need to add .vue-form-generator to the fieldset and change the selectors in the styles.

For multiple-top-level elements: fix it later. Just collect the concerned fields and create an issue.

@@ -54,6 +54,7 @@
"eslint-loader": "1.5.0",
"eslint-plugin-html": "1.5.1",
"eslint-plugin-vue": "0.1.1",
"extract-text-webpack-plugin": "^1.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

Please change to static version number "v1.0.1"

@coveralls
Copy link

coveralls commented Aug 9, 2016

Coverage Status

Coverage remained the same at 87.245% when pulling 778a9ff on lionel-bijaoui:LB_extract_style into 66d201d on icebob:master.

@icebob
Copy link
Member

icebob commented Aug 9, 2016

@lionel-bijaoui do you need my help for styles?

@lionel-bijaoui
Copy link
Member Author

@icebob it's ok for me, I have a few questions to be clear on what direction you want to go.

@lionel-bijaoui
Copy link
Member Author

@icebob What should be the rule regarding bootstrap specific class (.form-control, .form-group, .form-inline, .form-horizontal, .radio, .radio-inline, .checkbox, .checkbox-inline, .form-control-static, etc) ?
Should we go full bootstrap way or keep only a minimal list of class?

@coveralls
Copy link

coveralls commented Aug 9, 2016

Coverage Status

Coverage remained the same at 87.245% when pulling 49e8d43 on lionel-bijaoui:LB_extract_style into 66d201d on icebob:master.

@icebob
Copy link
Member

icebob commented Aug 9, 2016

@lionel-bijaoui we add minimal classes from bootstrap (e.g. .form-control, .form-group)
So the "official" style/theme would be bootstrap theme for input and button fields

@icebob
Copy link
Member

icebob commented Aug 9, 2016

Are you still working on this PR?

@@ -1,5 +1,5 @@
<template lang="jade">
fieldset(v-if="schema != null")
fieldset.vue-form-generator(v-if="schema != null")
Copy link
Member

Choose a reason for hiding this comment

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

This selector need to add in styles too:

<style lang="sass">

    $errorColor: lighten(#F00, 0%);

    fieldset.vue-form-generator {

@lionel-bijaoui
Copy link
Member Author

I'm almost ready. I will implement your comment and it should be good.

@icebob
Copy link
Member

icebob commented Aug 9, 2016

Great! 👍

@coveralls
Copy link

coveralls commented Aug 9, 2016

Coverage Status

Coverage remained the same at 87.245% when pulling b2fcef1 on lionel-bijaoui:LB_extract_style into 66d201d on icebob:master.

@lionel-bijaoui
Copy link
Member Author

@icebob Ready to merge ?

@icebob
Copy link
Member

icebob commented Aug 9, 2016

Firstly I create a branch based on this PR. Because I want to change styles on fieldSwitch too (remove white shadow because it is ugly on black background)

@icebob
Copy link
Member

icebob commented Aug 9, 2016

@icebob
Copy link
Member

icebob commented Aug 9, 2016

Merged in #29

@icebob icebob closed this Aug 9, 2016
@icebob
Copy link
Member

icebob commented Aug 9, 2016

@lionel-bijaoui Thanks for your help :) 👍

@lionel-bijaoui
Copy link
Member Author

Thank you, but I'm a bit lost, was I supposed to do something about my branch and your branch ?

@icebob
Copy link
Member

icebob commented Aug 9, 2016

No, it's ready. But I can't commit to your branch/PR, so I open an own branch what is based on your branch and made other changes in the styles.
E.g.: I added some bootstrap styles, so if bootstrap is not loaded, the looks will be same.
Check this fiddle: https://jsfiddle.net/icebob/0mg1v81e/
Now bootstrap removed from External links, add vue-form-generator.css and looks is similar! 👍

But I don't know what is this multiple empty lines in the generated css: https://github.com/icebob/vue-form-generator/blob/master/dist/vue-form-generator.css#L193

@lionel-bijaoui
Copy link
Member Author

Great ! I was a bit confused 😕
Can I delete the branch ?

@icebob
Copy link
Member

icebob commented Aug 9, 2016

Yes you can. Now your commits are in the master.

@lionel-bijaoui lionel-bijaoui deleted the LB_extract_style branch August 9, 2016 15:38
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.

3 participants