Skip to content

Conversation

SamThilmany
Copy link
Contributor

Added npm scripts to compile the scss
Added folders for the less and the scss code
Changed the path to the less version in the Gruntfile


You can now overwrite the image path by setting the $rel-mockup-path variable to the correct path, before importing the device-mockups.scss from the node_modules.

Added npm scripts to compile the scss
Added folders for the less and the scss code
Changed the path to the less version in the Gruntfile
@SamThilmany
Copy link
Contributor Author

SamThilmany commented Nov 13, 2017

Actually, the default value of the $rel-mockup-path variable should point to the actual folder of this project. I will change it to ../../device-mockups and recompile the css.

@ben182
Copy link
Collaborator

ben182 commented Nov 13, 2017

Hi SamThilmany,

I appreciate and checked ur changes. Actually I dont think this is helpful. Preprocessors are to help writing simpler CSS. If we put 2 different in the bucket it gets complicated. Moreover, with two preprocessors we have two device array files. This would make it more difficult to maintain.

Ben

@SamThilmany
Copy link
Contributor Author

@ben182 I only partly agree with your opinion.

Preprocessors are to help writing simpler CSS

Yes, absolutely, but they also give you the possibility to include the styles into your own project styles and overwrite some basic settings. Think on bootstrap for example. If you are building a serious website, you won't include the basic bootstrap stylesheet, but import those modules you need and alter the variables to your needs.

Okay, so you might think that what I just said only applies to huge projects such as bootstrap, but the simple fact, that I cannot change the path to the mockup images in your current code shows the opposite. If I want to use html5-device-mockups I have to have the same project structure as the one defined, otherwise, the css code will not find the images.

In my opinion, this is a huge shortcoming of the project. Have a look at font-awesome, they define a 'scss' and a 'less' variable for the path to the fonts too, otherwise, you wouldn't be able to use it, if you choose to import it yourself.

Moreover, with two preprocessors we have two device array files.

That's a big problem, I know. Maybe it is possible to write one array, that can be read from 'scss' and from 'less'. Hopefully, there is a possibility to do so.


I'm including all my css dependencies into my scss files, which is also commonly done in the industry. So why restrict this to less? The popularity of scss is growing and has overtaken less by far, so there is really no reason (besides the two arrays) to not include a scss version for those users that want to use it.

I really like the way font-awesome deals with this by offering the two most common preprocessors and a compiled version for those users that simply want to add a style tag.


Sidenote: This PR came to life because I wanted to use html5-device-mockups in one of my projects and I needed a scss version. That's why I quickly translated your less code into scss.


Regrads, Sam

@ben182
Copy link
Collaborator

ben182 commented Nov 14, 2017

Okay, so I sleeped about it and I guess I accept ur PR. I think I can live with two preprocessors and since we get more developers into the project with scss its a win-win for all. The problem with the doubled maintenance is still present but I dont know a solution to this yet. Actually we have to keep in mind that we have two files to maintain.

@ben182 ben182 merged commit babc696 into pixelsign:master Nov 14, 2017
@SamThilmany
Copy link
Contributor Author

Great to see that this was accepted.
Just as a side note: If you add me as Collaborator/Developer I'll happily take care of the scss in the future.

@ben182
Copy link
Collaborator

ben182 commented Nov 14, 2017

U know what? I consider removing the less part. When I look over the new scss files in comparison, I notice the more simple and more beautiful approach. Especially for the loop.

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.

2 participants