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

Scss normalization #6372

Merged
merged 7 commits into from
Sep 25, 2017
Merged

Scss normalization #6372

merged 7 commits into from
Sep 25, 2017

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Sep 5, 2017

Processus:

  1. Set the variables to
$color-main-text: #e2e2e2;
$color-main-background: #242424;
$color-primary: #0082c9;
$color-primary-text: #ffffff;
$color-error: #e9322d;
$color-warning: #ffcc44;
$color-success: #46ba61;
$color-primary-element: $color-primary;

@function nc-darken($color, $value) {
  @return lighten($color, $value);
}

@function nc-lighten($color, $value) {
  @return darken($color, $value);
}
  1. Browse nextcloud and detect inconsistencies! :)

This pr includes:

@skjnldsv skjnldsv added 2. developing Work in progress design Design, UI, UX, etc. labels Sep 5, 2017
@skjnldsv skjnldsv self-assigned this Sep 5, 2017
@@ -90,7 +90,7 @@ kbd {
-moz-user-select: none;
-ms-user-select: none;
user-select: none;
border-right: 1px solid nc-darken($color-main-background, 8%);
border-right: 1px solid $color-border;
Copy link
Member

Choose a reason for hiding this comment

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

Where does the $color-border comes from?

Copy link
Member Author

Choose a reason for hiding this comment

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

Newly created var for all our borders in nc! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a WIP, still haven't pushed the variables.scss 👍

@codecov
Copy link

codecov bot commented Sep 6, 2017

Codecov Report

Merging #6372 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #6372      +/-   ##
============================================
- Coverage     53.06%   53.06%   -0.01%     
- Complexity    22553    22567      +14     
============================================
  Files          1414     1417       +3     
  Lines         87745    87800      +55     
  Branches       1340     1340              
============================================
+ Hits          46565    46594      +29     
- Misses        41180    41206      +26
Impacted Files Coverage Δ Complexity Δ
apps/files_trashbin/lib/Expiration.php 90.32% <0%> (-1.62%) 29% <0%> (ø)
...av/lib/Migration/Version1004Date20170924124212.php 0% <0%> (ø) 1% <0%> (?)
...k/Middleware/Security/SameSiteCookieMiddleware.php 51.16% <0%> (ø) 12% <0%> (?)
...ty/Exceptions/LaxSameSiteCookieFailedException.php 100% <0%> (ø) 1% <0%> (?)
lib/base.php 2.91% <0%> (+0.01%) 172% <0%> (-1%) ⬇️
apps/dav/lib/CardDAV/CardDavBackend.php 85.56% <0%> (+0.03%) 89% <0%> (+1%) ⬆️
...e/AppFramework/DependencyInjection/DIContainer.php 77.72% <0%> (+0.59%) 27% <0%> (ø) ⬇️

@skjnldsv
Copy link
Member Author

skjnldsv commented Sep 6, 2017

@nextcloud/designers what do you think of this?
@MorrisJobke @juliushaertl @jancborchardt
kazam_screenshot_00002kazam_screenshot_00003

I always find the lack of feedback on the settings button disturbing, considering we have a unified design for all the links above :)

@jancborchardt
Copy link
Member

@skjnldsv good call! Totally didn't occur to me :D

(I guess you are already thinking/working how to make the sidebar icons adapt to the background if in dark mode? ;) cc @juliushaertl

@skjnldsv
Copy link
Member Author

skjnldsv commented Sep 7, 2017

@jancborchardt The switch to dark is the beginning of my whole work on the scss implementation, It never left my todo list!! :D

Look at #1692 ;)

@skjnldsv
Copy link
Member Author

Rebased to latest master
Will continue the work!

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 21, 2017
@skjnldsv
Copy link
Member Author

I think we're good here! :)
Please review @nextcloud/designers

@MorrisJobke
Copy link
Member

I will review once #6607 is fixed.

@skjnldsv
Copy link
Member Author

@MorrisJobke Your turn! ;)

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Changes look good and i could not find any visual issues with it. 👍

By the way, it would be awesome if you could clean up the commit history for this a bit. Either squash it into one or even better group them somehow logically together.

@skjnldsv
Copy link
Member Author

@juliushaertl why? :)

@juliusknorr
Copy link
Member

@skjnldsv In general they are fine, since they are already nicely grouped by small changes. The main issue i have with those is, that most of the commit messages won't help you to get what this commit is about, when you read them without context. Makes it kind of hard to track down issues afterwords imo. Maybe just a personal thing.

@skjnldsv
Copy link
Member Author

@juliushaertl I see your point! :)
I can squash all of it since this is a simple pr!

variables.

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Scss conversion
Cleanup settings scss

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
@skjnldsv
Copy link
Member Author

@juliushaertl all clear! ;)

@juliusknorr
Copy link
Member

Awesome, thanks. 👍 that also makes it easier to review and checkout the changes.

@eppfel eppfel mentioned this pull request Sep 24, 2017
13 tasks
@MorrisJobke
Copy link
Member

But somehow JSUnit fails:

PhantomJS 2.1.1 (Linux 0.0.0) OCA.Files.FileList tests Renaming files Restores thumbnail when rename was cancelled FAILED
	Expected 'http://localhost/core/img/filetypes/text.svg' to equal 'http://localhost/core/img/loading.gif'.
	apps/files/tests/js/filelistSpec.js:753:13
PhantomJS 2.1.1 (Linux 0.0.0) OCA.Files.FileList tests Moving files Restores thumbnail if a file could not be moved FAILED
	Expected 'http://localhost/core/img/filetypes/text.svg' to equal 'http://localhost/core/img/loading.gif'.
	apps/files/tests/js/filelistSpec.js:841:13
PhantomJS 2.1.1 (Linux 0.0.0) OCA.Files.FileList tests Copying files Restores thumbnail if a file could not be copied FAILED
	Expected 'http://localhost/core/img/filetypes/text.svg' to equal 'http://localhost/core/img/loading.gif'.
	apps/files/tests/js/filelistSpec.js:938:13
PhantomJS 2.1.1 (Linux 0.0.0) OCA.Files.FileList tests showFileBusyState shows spinner on busy rows FAILED
	Expected 'http://localhost/core/img/filetypes/image.svg' to equal 'http://localhost/core/img/loading.gif'.
	apps/files/tests/js/filelistSpec.js:3153:13

@skjnldsv
Copy link
Member Author

Let me take a look @MorrisJobke

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
@skjnldsv
Copy link
Member Author

@MorrisJobke Tests fixed. Let's wait for drone! 🎉

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

🚀

@skjnldsv skjnldsv merged commit 9080976 into master Sep 25, 2017
@skjnldsv skjnldsv deleted the scss-normalization branch September 25, 2017 14:50
@MorrisJobke MorrisJobke added this to the Nextcloud 13 milestone Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UI, UX, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

app settings dialog visibility should be increased
4 participants