Bug 19474

Summary: Convert staff client CSS to SCSS
Product: Koha Reporter: Owen Leonard <oleonard>
Component: TemplatesAssignee: Owen Leonard <oleonard>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: enhancement    
Priority: P5 - low CC: claire.hernandez, claire_gravely, dilan, gmcharlt, jonathan.druart, josef.moravec, katrin.fischer, kyle, martin.renvoize, mtj, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
URL: https://wiki.koha-community.org/wiki/Working_with_staff_client_SCSS
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15522
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20329
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=1918
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 20217, 20427, 21195, 21223, 21229, 21237, 21239, 21363, 21470, 23572    
Attachments: Bug 19474: Convert staff client CSS to SCSS
Bug 19474 [follow-up] Convert staff client CSS to SCSS
Bug 19474 (Follow-up) Convert staff client CSS to SCSS
[SIGNED-OFF] Bug 19474: Convert staff client CSS to SCSS
[SIGNED-OFF] Bug 19474 [follow-up] Convert staff client CSS to SCSS
[SIGNED-OFF] Bug 19474 (Follow-up) Convert staff client CSS to SCSS
Bug 19474: Convert staff client CSS to SCSS
Bug 19474: (follow-up) Convert staff client CSS to SCSS
Bug 19474: (follow-up) Convert staff client CSS to SCSS
Bug 19474: (QA follow-up) Fix sass lint yaml configuration
Bug 19474: Convert staff client CSS to SCSS
Bug 19474: Convert staff client CSS to SCSS
Bug 19474: (follow-up) Convert staff client CSS to SCSS
Bug 19474: (follow-up) Convert staff client CSS to SCSS
Bug 19474: (QA follow-up) Fix sass lint yaml configuration
Bug 19474: Convert staff client CSS to SCSS
Bug 19474: (follow-up) Convert staff client CSS to SCSS
Bug 19474: (follow-up) Convert staff client CSS to SCSS
Bug 19474: (QA follow-up) Fix sass lint yaml configuration
Bug 19474: (follow-up) Catch up with changes to staff-global.css
Bug 19474 (follow-up) Catch up with changes to CSS from Bug 19981
Bug 19474: (follow-up) Catch up with changes to CSS from Bug 19981
Bug 19474: (follow-up) Catch up with changes to CSS from Bug 19981
Bug 19474: (follow-up) Catch up with changes to staff-global.css
Bug 19474: (follow-up) Catch up with changes to CSS from Bug 19981
Bug 19474: (follow-up) Catch up with changes to staff-global.css
Bug 19474: Convert staff client CSS to SCSS
Bug 19474: (QA follow-up) Fix sass lint yaml configuration
Bug 19474 (follow-up) Compiled CSS
Bug 19474: Convert staff client CSS to SCSS
Bug 19474: (QA follow-up) Fix sass lint yaml configuration
Bug 19474 (follow-up) Compiled CSS
Bug 19474: (follow-up) Compiled CSS
Bug 19474: Convert staff client CSS to SCSS
Bug 19474: (QA follow-up) Fix sass lint yaml configuration
Bug 19474 (follow-up) Compiled CSS
Bug 19474: (follow-up) Bring scss up to date with latest CSS changes
Bug 19474: (follow-up) Compiled CSS
Bug 19474: (follow-up) Bring scss up to date with latest CSS changes
Bug 19474: (follow-up) Compiled CSS
Bug 19474: (follow-up) Bring scss up to date with latest CSS changes
Bug 19474: (follow-up) Bring scss up to date with latest CSS changes
Bug 19474: (follow-up) Bring scss up to date with latest CSS changes

Description Owen Leonard 2017-10-17 12:56:29 UTC
I propose a new workflow for handling CSS in the staff client based on the work in  Bug 15522 which adds a client-side asset processing workflow. My patch will be designed to work independently from that bug but use the same framework so that the two can be merged easily.

- Uses yarn (https://yarnpkg.com) to manage installation of npm dependencies
- Uses gulp (https://gulpjs.com) to process client-side assets. In this case, conversion of SCSS to CSS, auto-prefixing of CSS, minification of CSS.

This patch will add a configuration file for sass-lint (https://github.com/sasstools/sass-lint) to help standardize how we write SCSS. The version I'm working with is pretty much the default, and I hope it won't be controversial. I'm using this configuration file in conjunction with my editor's linting plugin. We might later plug sass-lint into the gulp build process.

The patch will include a conversion of staff-global.css to SCSS and remove staff-global.css from git.
Comment 1 Owen Leonard 2017-10-17 13:20:17 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2017-10-17 13:22:29 UTC
Note to testers: The qa tools gave me a fail for .sass-lint.yml ("YAML Error: Invalid element in map"), but using web-based YAML linter returned no errors. Not sure what's going on there.
Comment 3 Jonathan Druart 2017-10-17 13:35:30 UTC
(In reply to Owen Leonard from comment #2)
> Note to testers: The qa tools gave me a fail for .sass-lint.yml ("YAML
> Error: Invalid element in map"), but using web-based YAML linter returned no
> errors. Not sure what's going on there.

The script executes YAML::LoadFile($filepath) and displays the lines containing "YAML error".
Comment 4 Owen Leonard 2017-10-17 13:51:22 UTC
Another note to testers: If you are using the included sass-lint configuration to lint the new scss file, it's not perfect. I've tried to make a good first pass as properly nesting staff-global.css, but there are many cases where I'm not sure using strict SCSS syntax results in more efficient output than the non-nested version.
Comment 5 Dilan Johnpullé 2017-10-26 01:01:34 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2017-10-30 12:12:26 UTC
This patch deletes staff-global.css, assuming that staff-global.css should be generated from the SCSS file. Since commits have been made to stafff-global.css in the meantime, it creates a conflict. I will submit follow-ups to catch up with master, but I think this could be properly tested by resolving the conflict manually.
Comment 7 Owen Leonard 2017-11-17 14:07:11 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2018-01-05 19:48:28 UTC Comment hidden (obsolete)
Comment 9 Claire Gravely 2018-01-25 15:22:29 UTC Comment hidden (obsolete)
Comment 10 Claire Gravely 2018-01-25 15:22:33 UTC Comment hidden (obsolete)
Comment 11 Claire Gravely 2018-01-25 15:22:36 UTC Comment hidden (obsolete)
Comment 12 Josef Moravec 2018-01-26 10:00:11 UTC Comment hidden (obsolete)
Comment 13 Josef Moravec 2018-01-26 10:00:14 UTC Comment hidden (obsolete)
Comment 14 Josef Moravec 2018-01-26 10:00:17 UTC Comment hidden (obsolete)
Comment 15 Josef Moravec 2018-01-26 10:00:20 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2018-01-30 07:18:03 UTC
Noticing this is PQA :) Should we prepare a wiki page with instructions to go with it?
Comment 17 Kyle M Hall 2018-01-30 13:28:54 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2018-01-30 13:29:43 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2018-01-30 13:29:51 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2018-01-30 13:29:54 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2018-01-30 13:29:57 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2018-01-30 13:31:30 UTC
(In reply to Katrin Fischer from comment #16)
> Noticing this is PQA :) Should we prepare a wiki page with instructions to
> go with it?

Yes, that seems like an excellent idea.
Comment 24 Jonathan Druart 2018-01-30 16:41:01 UTC
.gitignore is not tracked yet, so I get:

error: The following untracked working tree files would be overwritten by merge:
        .gitignore
Please move or remove them before you merge.
Comment 25 Kyle M Hall 2018-01-30 17:22:07 UTC
(In reply to Jonathan Druart from comment #24)
> .gitignore is not tracked yet, so I get:
> 
> error: The following untracked working tree files would be overwritten by
> merge:
>         .gitignore
> Please move or remove them before you merge.

Sounds like you added your own .gitignore file. Just remove it and everything should apply just fine. You could add a followup to merge the two versions if what you have in yours would be generally useful.
Comment 26 Jonathan Druart 2018-01-30 17:37:12 UTC
I thought it was not a good idea to commit the .gitignore file, but actually it does.
.git/info/exclude must be used for local modifications.
Comment 27 Jonathan Druart 2018-01-30 19:20:53 UTC
Did we discuss and vote this change in a dev meeting or the mailing list (I do not find track of that)?

I do not understand why the .css is no longer part of the codebase. Do not you think RM should generate it?
Comment 28 Owen Leonard 2018-01-30 19:30:13 UTC
I think most of the discussion so far centered around a different bug, Bug 15522.
We discussed the idea of implementing this kind of workflow on the mailing list here:

http://lists.koha-community.org/pipermail/koha-devel/2017-August/043910.html

...and during a dev meeting:

http://meetings.koha-community.org/2017/development_irc_meeting_9_august_2017.2017-08-09-22.00.log.html
Comment 29 Jonathan Druart 2018-02-08 17:49:37 UTC
(In reply to Jonathan Druart from comment #27)
> I do not understand why the .css is no longer part of the codebase. Do not
> you think RM should generate it?

?
Comment 30 Owen Leonard 2018-02-14 14:40:57 UTC
Created attachment 71609 [details] [review]
Bug 19474: Convert staff client CSS to SCSS

This patch converts staff-global.css to Sass, using SCSS syntax. This
changes the build process for Koha to include installation and execution
of yarn to install npm dependencies and run SCSS -> CSS conversion.

To test, apply the patch and run the following:

$ sudo apt-get install nodejs npm [not necessary in kohadevbox]
$ sudo npm install -g yarn
$ yarn install
$ yarn build

Clear your browser cache if necessary and confirm that CSS styling
throughout the staff client looks correct.

The "yarn build" command triggers a gulp process which compiles SCSS to
CSS, adds automatic vendor-prefixing, and minifies the resulting CSS
file.

There is also a "yarn css" command available which might be used by
developers who are making changes to SCSS. This command does two things
differently:

1. Adds .css.map files which aid CSS debugging using in-browser
   inspector tools.
2. Compiles staff-global.css without minification. It can be useful to
   see unminified CSS during development, especially to see how SCSS
   compiles.

This patch adds a configuration file for sass-lint, .sass-lint.yml.
Currently this configuration is not used during the build process but
can be used in a code editor which supports linting.

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Owen Leonard 2018-02-14 14:41:00 UTC Comment hidden (obsolete)
Comment 32 Owen Leonard 2018-02-14 14:41:34 UTC Comment hidden (obsolete)
Comment 33 Owen Leonard 2018-02-14 14:41:46 UTC Comment hidden (obsolete)
Comment 34 Owen Leonard 2018-02-14 14:45:39 UTC
I've modified the first patch in two ways:

1. staff-global.css now remains in git
2. .gitignore no longer ignores staff-global.css

QA team: Should I seek another signoff?
Comment 35 Owen Leonard 2018-02-14 15:53:21 UTC Comment hidden (obsolete)
Comment 36 Owen Leonard 2018-03-05 19:38:01 UTC Comment hidden (obsolete)
Comment 37 Owen Leonard 2018-03-12 10:58:02 UTC Comment hidden (obsolete)
Comment 38 Owen Leonard 2018-03-12 12:49:32 UTC Comment hidden (obsolete)
Comment 39 Claire Gravely 2018-03-12 13:28:26 UTC Comment hidden (obsolete)
Comment 40 Claire Gravely 2018-03-12 13:28:30 UTC Comment hidden (obsolete)
Comment 41 Claire Gravely 2018-03-12 13:32:11 UTC Comment hidden (obsolete)
Comment 42 Owen Leonard 2018-03-13 14:01:21 UTC Comment hidden (obsolete)
Comment 43 Owen Leonard 2018-03-13 14:02:26 UTC Comment hidden (obsolete)
Comment 44 Owen Leonard 2018-03-13 14:08:03 UTC Comment hidden (obsolete)
Comment 45 Katrin Fischer 2018-03-13 15:05:23 UTC Comment hidden (obsolete)
Comment 46 Katrin Fischer 2018-03-13 15:05:29 UTC Comment hidden (obsolete)
Comment 47 Katrin Fischer 2018-03-13 15:05:34 UTC Comment hidden (obsolete)
Comment 48 Katrin Fischer 2018-03-13 15:06:59 UTC Comment hidden (obsolete)
Comment 49 Owen Leonard 2018-03-30 12:07:50 UTC
Created attachment 73470 [details] [review]
Bug 19474: Convert staff client CSS to SCSS

This patch converts staff-global.css to Sass, using SCSS syntax. This
changes the build process for Koha to include installation and execution
of yarn to install npm dependencies and run SCSS -> CSS conversion.

To test, apply the patch and run the following:

$ sudo apt-get install nodejs npm [not necessary in kohadevbox]
$ sudo npm install -g yarn
$ yarn install
$ yarn build

Clear your browser cache if necessary and confirm that CSS styling
throughout the staff client looks correct.

The "yarn build" command triggers a gulp process which compiles SCSS to
CSS, adds automatic vendor-prefixing, and minifies the resulting CSS
file.

There is also a "yarn css" command available which might be used by
developers who are making changes to SCSS. This command does two things
differently:

1. Adds .css.map files which aid CSS debugging using in-browser
   inspector tools.
2. Compiles staff-global.css without minification. It can be useful to
   see unminified CSS during development, especially to see how SCSS
   compiles.

This patch adds a configuration file for sass-lint, .sass-lint.yml.
Currently this configuration is not used during the build process but
can be used in a code editor which supports linting.

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 50 Owen Leonard 2018-03-30 12:07:54 UTC
Created attachment 73471 [details] [review]
Bug 19474: (QA follow-up) Fix sass lint yaml configuration

Test plan:
QA tools should report green on the .sass-lint.yml file now

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 51 Owen Leonard 2018-03-30 12:12:03 UTC Comment hidden (obsolete)
Comment 52 Jonathan Druart 2018-04-18 20:05:46 UTC
Could we imagine to have a staff-global.css and staff-global.min.css 
It would depend on the dev_install config flag, devs will use .css whereas production installations will use .min.css

The .min.css would be generated only on releasing (and will avoid to pollute the git log).
Comment 53 Jonathan Druart 2018-04-18 20:13:53 UTC
Owen, could you add, on the wiki page, a list of the files bug 19474 and bug 20427 are going to add to our codebase? And tell what is the purpose in front each one.
Comment 54 Jonathan Druart 2018-04-18 20:15:22 UTC
We should also update https://wiki.koha-community.org/wiki/Working_with_Bootstrap_OPAC_LESS_files
Comment 55 Owen Leonard 2018-04-23 13:26:26 UTC
Created attachment 74724 [details] [review]
Bug 19474: (follow-up) Bring scss up to date with latest CSS changes

This patch catches the scss up to the latest changes in master:

Bug 19946, Bug 20045, Bug 20343, Bug 18327, Bug 20322, and Bug 18799
Comment 56 Owen Leonard 2018-04-23 13:26:31 UTC Comment hidden (obsolete)
Comment 57 Owen Leonard 2018-04-23 17:49:01 UTC
Created attachment 74757 [details] [review]
Bug 19474: (follow-up) Bring scss up to date with latest CSS changes

This patch catches the scss up to the latest changes in master:

Bug 19946, Bug 20045, Bug 20343, Bug 18327, Bug 20322, Bug 18799, and
Bug 20622.
Comment 58 Owen Leonard 2018-04-23 17:49:05 UTC
Created attachment 74758 [details] [review]
Bug 19474: (follow-up) Compiled CSS

This patch includes the compiled CSS.
Comment 59 Jonathan Druart 2018-04-25 16:12:20 UTC
I am stuck here and I need help:
https://framabin.org/?92fc14016eb1b26b#Ign1IDycTejFZj7suueyIXO/wEy2wFB13hxbqq5Yle0=

I changed .scss file and `yarn build` or `yarn css` does not update .css files.
Comment 60 Jonathan Druart 2018-05-04 12:21:02 UTC
This will not be part of 18.05 unfortunately.
Comment 61 Owen Leonard 2018-07-03 19:44:01 UTC
Created attachment 76657 [details] [review]
Bug 19474: (follow-up) Bring scss up to date with latest CSS changes

This patch catches the scss up to the latest changes in master:

Bug 19946
Bug 20045
Bug 20343
Bug 18327
Bug 20322
Bug 18799
Bug 20622
Bug 20329
Bug 20744
Bug 20931
Bug 20793
Bug 20651
Comment 62 Owen Leonard 2018-07-23 16:24:47 UTC
Created attachment 77189 [details] [review]
Bug 19474: (follow-up) Bring scss up to date with latest CSS changes

This patch catches the scss up to the latest changes in master:

Bug 19946
Bug 20045
Bug 20343
Bug 18327
Bug 20322
Bug 18799
Bug 20622
Bug 20329
Bug 20744
Bug 20931
Bug 20793
Bug 20651
Comment 63 Jonathan Druart 2018-07-23 17:18:39 UTC
(In reply to Jonathan Druart from comment #59)
> I am stuck here and I need help:
> https://framabin.org/?92fc14016eb1b26b#Ign1IDycTejFZj7suueyIXO/
> wEy2wFB13hxbqq5Yle0=
> 
> I changed .scss file and `yarn build` or `yarn css` does not update .css
> files.

Retested right now and I do see the expected changes made to koha-tmpl/intranet-tmpl/prog/css/staff-global.css
Comment 64 Jonathan Druart 2018-07-23 17:29:56 UTC
(In reply to Jonathan Druart from comment #63)
> (In reply to Jonathan Druart from comment #59)
> > I am stuck here and I need help:
> > https://framabin.org/?92fc14016eb1b26b#Ign1IDycTejFZj7suueyIXO/
> > wEy2wFB13hxbqq5Yle0=
> > 
> > I changed .scss file and `yarn build` or `yarn css` does not update .css
> > files.
> 
> Retested right now and I do see the expected changes made to
> koha-tmpl/intranet-tmpl/prog/css/staff-global.css

I would like to retest on top of bug 20427 as well (maybe it came from there?), but it does no longer apply.
Comment 65 Jonathan Druart 2018-07-23 17:42:25 UTC
(In reply to Jonathan Druart from comment #64)
> (In reply to Jonathan Druart from comment #63)
> > (In reply to Jonathan Druart from comment #59)
> > > I am stuck here and I need help:
> > > https://framabin.org/?92fc14016eb1b26b#Ign1IDycTejFZj7suueyIXO/
> > > wEy2wFB13hxbqq5Yle0=
> > > 
> > > I changed .scss file and `yarn build` or `yarn css` does not update .css
> > > files.
> > 
> > Retested right now and I do see the expected changes made to
> > koha-tmpl/intranet-tmpl/prog/css/staff-global.css
> 
> I would like to retest on top of bug 20427 as well (maybe it came from
> there?), but it does no longer apply.

Confirmed! The issue still exists for me with bug 20427 applied
Comment 66 Jonathan Druart 2018-07-23 19:06:12 UTC
(In reply to Jonathan Druart from comment #65)
> (In reply to Jonathan Druart from comment #64)
> > (In reply to Jonathan Druart from comment #63)
> > > (In reply to Jonathan Druart from comment #59)
> > > > I am stuck here and I need help:
> > > > https://framabin.org/?92fc14016eb1b26b#Ign1IDycTejFZj7suueyIXO/
> > > > wEy2wFB13hxbqq5Yle0=
> > > > 
> > > > I changed .scss file and `yarn build` or `yarn css` does not update .css
> > > > files.
> > > 
> > > Retested right now and I do see the expected changes made to
> > > koha-tmpl/intranet-tmpl/prog/css/staff-global.css
> > 
> > I would like to retest on top of bug 20427 as well (maybe it came from
> > there?), but it does no longer apply.
> 
> Confirmed! The issue still exists for me with bug 20427 applied

Fixed since patch on bug 20427 comment 13.
Comment 67 Owen Leonard 2018-08-09 13:15:25 UTC
Created attachment 77621 [details] [review]
Bug 19474: (follow-up) Bring scss up to date with latest CSS changes

This patch catches the scss up to the latest changes in master:

Bug 19946
Bug 20045
Bug 20343
Bug 18327
Bug 20322
Bug 18799
Bug 20622
Bug 20329
Bug 20744
Bug 20931
Bug 20793
Bug 20651
Bug 20668
Bug 16575
Bug 19166
Bug 20770
Bug 21163
Bug 20779
Comment 68 Nick Clemens 2018-08-09 15:26:33 UTC
Awesome work all!

Pushed to master for 18.11
Comment 69 Martin Renvoize 2018-08-10 08:22:25 UTC
Enhancement, this will not be backported to 18.05.x series.
Comment 70 Jonathan Druart 2018-08-16 18:56:29 UTC
I have opened bug 21239 to list the different regressions introduced by this change.
Comment 71 Martin Renvoize 2018-11-12 09:38:56 UTC
Any chance of a short rationale and summary for the release notes for this one Owen?
Comment 72 Owen Leonard 2019-03-05 18:43:21 UTC
*** Bug 11896 has been marked as a duplicate of this bug. ***