search

kudago / smart-app-banner

Lightweight smart app banner with no jquery requirement
MIT License
526 stars 250 forks source link

Bring smart banners into 2019 #113

Open tomascordero opened 5 years ago

tomascordero commented 5 years ago

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 commented 5 years ago

image :grimacing:

tomascordero commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

DaSchTour commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

tomascordero commented 5 years ago

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.