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

Bring smart banners into 2019 #113

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

Conversation

tomascordero
Copy link

I did somewhat of a bigger refactor of the styles.

For the CSS:

  1. I migrated the CSS to SCSS for easier maintenance.
    -- This means I created a "sass" folder containing all sass.
  2. I added gulp to the build process to compile the SCSS
  3. I added a test command npm run test to start a small test environment for dev
  4. I brought all the banners up to date with current design trends using google's material design.
  5. I added Flexbox

As for the Javascript:

Not much changed in this. I just added a few classes to the button for the material design. One of those classes is a color class that could be added to the options.

I hope these changes are welcome changes! I think the plugin is a great one and would love to see it brought up to current design trends.

Thanks,
Tomas

@dy
Copy link
Member

dy commented Jan 16, 2019

image 😬

@tomascordero
Copy link
Author

tomascordero commented Jan 16, 2019

image 😬

I know its a lot of changes but It adds a lot of future functionality (material design css library) and keeps this thing in line with the times!

@tomascordero
Copy link
Author

I guess we could remove some of those lines by removing some of the files that arent really needed. I just added the whole library. Im not pulling in all the modules when I compile I just have them in the repo

@dy
Copy link
Member

dy commented Jan 19, 2019

20 times more code... I'd expect ~10 lines of CSS changed to modernize it. This is a definitely a matter of a separate package.

@tomascordero
Copy link
Author

Let me revise it a bit. Ill remove the unused sass and make sure the included packages don't inject any unneeded CSS

@DaSchTour
Copy link

DaSchTour commented Jan 21, 2019

CSS library should not be added by copy but as a dependency. And it should be ensured, that only the needed SCSS is imported.

But I think this violates the idea of beeing standalone. So I'm not sure if it is really 2019 to add an external style library. It's definitly not 2019 to do it by copy.

@tomascordero
Copy link
Author

Im not sure what you mean by copy? I left the process the same exact way you had it to install the plugin to a website. As for the SCSS I only am importing the needed files however I believe there are some resets that are making their way over. I will clean those up.

Are you saying you want to change the way the CSS is included to a project?

@DaSchTour
Copy link

DaSchTour commented Jan 23, 2019

From what I understand the CSS is from an external library, so this external library is a dependency that needs to be included through package.json and imported from node_modules and they should not be commitet to git this repo.
And badges, cards, carousel, chips, datapicker, dropdown, grid and so on are not needed for this banner, so they should not be included anywhere in this project.
There are a lot of SCSS files pasted into this project that are not needed and that never had been pasted here as they should be included through importing from the place they were copied from.

@tomascordero
Copy link
Author

I understand what your saying! I will include them in the build process instead!

@tomascordero
Copy link
Author

Alright I went ahead and changed some stuff up with the sass. I also added a minified version of the css. Let me know what you think of it now.

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