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

Move to SCSS #219

Merged
merged 6 commits into from
Jun 30, 2017
Merged

Move to SCSS #219

merged 6 commits into from
Jun 30, 2017

Conversation

juliusknorr
Copy link
Member

I'll wait for #215 and try to restructure the css file then to make use of all the great features. 🎉

Fallback style.css will be generated when running make so it will keep working when using Nextcloud 11.

@codecov
Copy link

codecov bot commented Jun 27, 2017

Codecov Report

Merging #219 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #219   +/-   ##
=======================================
  Coverage   76.56%   76.56%           
=======================================
  Files          33       33           
  Lines         973      973           
=======================================
  Hits          745      745           
  Misses        228      228

@pixelipo
Copy link
Contributor

style.css is created but for some reason not loaded when I run the app...

Plus it doesn't keep line break before } closing brackets.

P.S. I can take care of converting to 'proper' scss, if you don't mind.

@juliusknorr
Copy link
Member Author

style.css is created but for some reason not loaded when I run the app...

For Nextcloud >12 it will generate the file automatically and load it from something like /index.php/css/deck/37d007a56d816107ce5b52c10342db37-style.css

Plus it doesn't keep line break before } closing brackets.

I guess that can be adjusted by using --output-style extended in the Makefile. But we should use compressed there anyway so we have a small file in the end.

P.S. I can take care of converting to 'proper' scss, if you don't mind.

That would be great.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Copy link
Member

@artemanufrij artemanufrij left a comment

Choose a reason for hiding this comment

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

icon is not visible:

screenshot_20170627_213404

@juliusknorr
Copy link
Member Author

icon is not visible:

That is basically because the Nextclouds scss functionality only rewrites urls that are using quotes like url('../../iconfile') and not url(../../iconfile). We should add quotes everywhere for now. Maybe @pixelipo can do this when he does the migration to scss. I'll create a PR in nextcloud/server to fix this later as well.

@pixelipo
Copy link
Contributor

No problem.

BTW, I have already grouped all styles, tomorrow I'll extract colors into variables and push here. That should be enough for first version of scss.

@artemanufrij
Copy link
Member

@juliushaertl I added quotes in pr #220

Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>
Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>
@pixelipo
Copy link
Contributor

@juliushaertl @artemanufrij please test this. I have converted to SCSS structure and added color variables. Also cleaned up few unused styles and simplified color palette (we literally had "50 shades of gray" 🤣 )

@artemanufrij
Copy link
Member

artemanufrij commented Jun 30, 2017

@pixelipo I will test it today afternoon.

@pixelipo
Copy link
Contributor

We can also close #160 once we merge this. There is still some unused CSS left, but it's not too bad and it can be cleaned up when needed.

@juliusknorr
Copy link
Member Author

Pushed another commit to small issues with the board list. Otherwise looks great 👍

Awesome work @pixelipo. Thanks for that.

css/style.scss Outdated
}

td {
padding: 0 10px;
padding: 3px 10px;
Copy link
Contributor

Choose a reason for hiding this comment

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

@juliushaertl please revert this line to padding: 0 10px;

If you think rows must be 50px high, then you can increase this to 25px: #boardlist .icon { padding: 25px; }

The difference between these 2 solutions is that with the latter, buttons fill full height of the row.

P.S. In my opinion, 44px is enough for row height anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

Copy link
Member

@artemanufrij artemanufrij left a comment

Choose a reason for hiding this comment

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

Great work!

juliusknorr and others added 2 commits June 30, 2017 16:24
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Marin Treselj <marin.treselj@forlagshuset.no>
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.

3 participants