[frontend/theme] script for bootswatch theme compilation #931

Ananas120 · 2023-04-03T14:15:03Z

Script to compile (new) bootswatch theme

Usage :

Go to inginious/frontend/static/css/themes
Run make yeti to compile the yeti theme

CLAassistant · 2023-04-03T14:15:11Z

All committers have signed the CLA.

anthonygego

All the source files should be present so that at least the devs can rebuild the themes. We agreed previously to keep a submodule of bootswatch to be sure we are using a version compatible with our sources. It's probably also possible to keep the all process done by grunt only not to add additional development dependencies.

anthonygego · 2023-04-06T10:31:18Z

inginious/frontend/static/css/themes/dark_mode.css

This is the compiled version. The source file should be stored somewhere.

anthonygego · 2023-04-06T10:36:13Z

inginious/frontend/static/css/themes/Makefile

+	mv bootswatch/dist/$(theme)/bootstrap.css $(theme).css
+
+install:
+	git clone $BOOTSWATCH_URL


In this case we do not use a fixed version as discussed before. @nrybowski suggested to use a submodule to include the source at a given commit stage. We can probably add a third-party folder at the source root to store the submodule.

Besides, as discussed previously, it would probably be better to store the source files for yeti and the modified yeti dark mode into our repository (keeping their copyright of course) and only keep a submodule to bootstrap.

In all case we need to be able to rebuild the dark_mode.css file from the sources.

anthonygego · 2023-04-06T10:38:14Z

inginious/frontend/static/css/themes/Makefile

+BASE_THEME	= yeti
+
+compile:
+	if [ ! -d "bootswatch" ]; then make install; fi


It would be more convenient if all these steps could be carried out by grunt to avoid adding development dependencies. In case the yeti sources are provided in our repo the build configuration file should be provided too and can be modified to place the file in the appropriate locations.

anthonygego · 2023-04-06T10:49:06Z

inginious/frontend/templates/layout.html

@@ -21,7 +21,7 @@
        <!-- INGInious Style -->
        <link href="{{get_homepath(True)}}/static/css/INGInious-rtl.css?v2" rel="stylesheet">
    {% else %}
-        <link href="{{get_homepath(True)}}/static/css/bootstrap.min.css" rel="stylesheet">
+        <link href="{{get_homepath(True)}}/static/css/themes/default.min.css" rel="stylesheet">
        <!-- INGInious Style -->
        <link href="{{get_homepath(True)}}/static/css/INGInious.css?v2" rel="stylesheet">


Maybe we should also provide a specific version of this file for the different themes ?

Ananas120 · 2023-04-26T13:03:02Z

I have updated the workflow to remove the need of any third-party module by copying the Gruntfile + build directory from the bootswatch repo (which is enough for theme compilation)
I have also added the "darkly" theme with the "dracula" CodeMirror theme (not fully tested in inginious + theme not deployable yet)

I left the Makefile for simplicty but it is not required

anthonygego

I made comments based on reading the code only. Please fetch the files for Bootstrap 4 in the base repo first.

anthonygego · 2023-05-04T06:29:59Z

themes/package.json

+  "devDependencies": {
+    "@lodder/grunt-postcss": "^3.1.1",
+    "autoprefixer": "^10.4.12",
+    "bootstrap": "^5.2.3",


The current codebase still relies on Bootstrap 4. This is probably the first change to do (the first comment to treat).

anthonygego · 2023-05-04T06:30:40Z

themes/Gruntfile.js

+const autoprefixer = require('autoprefixer');
+const pkg = require('./package.json');
+
+const SWATCHES = [


This can probably be cleaned

anthonygego · 2023-05-04T06:32:40Z

themes/build/html/thumbnail.html

@@ -0,0 +1,125 @@
+<!DOCTYPE html>


We probably don't need to keep any thumbnail rendering feature. I guess ?

anthonygego · 2023-05-04T06:33:20Z

themes/test.html

@@ -0,0 +1,12 @@
+<!DOCTYPE html>


Test file should not be pushed into the repository

anthonygego · 2023-05-04T06:34:44Z

themes/Gruntfile.js

@@ -0,0 +1,266 @@
+'use strict';


We should keep a note here about the original author.

anthonygego · 2023-05-04T06:41:46Z

themes/Gruntfile.js

+
+const BUILD_DIR = 'build/';
+const DIST_DIR = 'themes/';
+const DOCS_DEST = 'docs/5/';


I think that we can delete all things related to docs as we don't aim at redistributing a full modified bootstrap copy.

anthonygego · 2023-05-04T06:43:20Z

.gitignore

@@ -23,7 +23,12 @@ venv
 inginious/tasks
 # Default backup directory
 inginious/backup
+# The themes files
+themes/node_modules


The themes folder can be put into the frontend one. This would make sense.

anthonygego · 2023-05-04T06:45:53Z

themes/Makefile

+OUTPUT_DIR	= ../inginious/frontend/static/css/themes
+
+compile:
+	if [ ! -d "node_modules" ]; then make install; fi


I actually don't ask this to be modified, but make supports dependencies handling. That's one of his core features.

anthonygego · 2023-05-04T06:47:11Z

themes/Makefile

+	if [ ! -d "node_modules" ]; then make install; fi
+	if [ ! -d "$(OUTPUT_DIR)/$(theme)" ]; then cp -r $(OUTPUT_DIR)/default $(OUTPUT_DIR)/$(theme); fi
+	./node_modules/grunt/bin/grunt swatch:$(theme)
+	mv $(THEMES_DIR)/$(theme)/bootstrap.min.css $(OUTPUT_DIR)/$(theme)/.


There should clearly be way to modify Gruntfile.js in order to compile all themes as the default action, and to copy the bootstrap.min.css in the appropriate folder.

This way, this whole Makefile would be kind of useless and compiling the themes would just consists in npm install and grunt.

anthonygego · 2023-05-04T06:48:56Z

themes/Makefile

+
+install:
+	npm install
+	npm install grunt-cli


You can add grunt-cli as a dependency in package.json.

Ananas120 · 2023-06-01T06:57:36Z

Here is the updated version shown last week :

The makefile has been removed as well as the test.html and the docs/ folder
The Gruntfile has been cleaned up
The command to compile theme is the grunt build:<theme>
The themes/codemirror/ contains all available themes from https://codemirror.net/5/themes, and are properly integrated in inginious (at least in darkly and default (yeti) themes).
The INGInious.css file is specific to each theme as some coloring styles had to be removed for the darkly theme (it should be checked if these coloring styles are really relevant in the regular yeti theme)
The _bootswatch.scss file from darkly has been modified to better fit high contrast (especially in the "warning" or "info" tags used multiple times in inginious buttons / side-bar)
The "profile.js" and "profile" page / logic and user_manager has been modified to add the color / codemirror-color themes the same way as the language. The list is automatically adapted according to the available themes in frontend/static/css/themes(/codemirror)
The "common.js" file has been modified to support "non-default" codemirror theme (by adding the "theme" key). The "Codemirror.colorize" method does not support custom theme, it is the reason why I have added the "replace(...)" line + a custom html tag "theme=..." to get the information on the codemirror-theme used.
The "codemirror/default.css" is an empty file as the default configuration (style) is already in "codemirror/main.css". However, this "main" file is required for all the codemirror themes, as the theme-specific files only defines coloring-styles (and not the other configurations).It is the reason why I have created a "default" empty file, such that it is automatically included in the available theme list (and it is empty as it does not overwrite the "main.css" default colors)

anthonygego

Do you think you can split this PR in :

One with the themes and inginious/frontend/static/css changes. (that is, essentially the imported files and building logic)
One with the remaining changes (that is, the effective integration into INGInious)

That would allow me to make sure my review is complete as I don't want to review all the imported CSS files, the review listing is a bit messy right now :-)

Do not delete this one now

anthonygego · 2023-07-07T10:01:02Z

inginious/frontend/app.py

@@ -175,6 +175,15 @@ def get_app(config):
    available_languages = {"en": "English"}
    available_languages.update(available_translations)

+    available_themes = [
+        theme_dir for theme_dir in os.listdir(os.path.join('inginious', 'frontend', 'static', 'css', 'themes'))
+        if theme_dir not in ('yeti', 'codemirror') and not theme_dir.startswith(('.', '_'))


Not sure yeti is required as an exception here.

anthonygego · 2023-07-07T10:04:46Z

inginious/frontend/app.py

@@ -175,6 +175,15 @@ def get_app(config):
    available_languages = {"en": "English"}
    available_languages.update(available_translations)

+    available_themes = [
+        theme_dir for theme_dir in os.listdir(os.path.join('inginious', 'frontend', 'static', 'css', 'themes'))


You can use inginious.get_root_path() here. This will avoid issues with the current working directory environment variable. Typically in a production setup where the inginious-webapp script is not in the same directory as the inginious package.

anthonygego · 2023-07-07T10:05:10Z

inginious/frontend/app.py

+        if theme_dir not in ('yeti', 'codemirror') and not theme_dir.startswith(('.', '_'))
+    ]
+    available_codemirror_themes= [
+        file.split('.')[0] for file in os.listdir(os.path.join('inginious', 'frontend', 'static', 'css', 'themes', 'codemirror'))


Same comment here for the path.

anthonygego · 2023-07-07T10:13:42Z

inginious/frontend/parsable_text.py

@@ -35,6 +35,8 @@ def run(self):
        if not self.content:
            translation = _get_inginious_translation()
            self.content = [translation.gettext("[no content]")]
+
+        self.options.setdefault('classes', []).append('theme={}'.format(flask.current_app.user_manager.get_codemirror_theme()))


That should be OK but maybe it's more careful and idiomatic (it seems to be the de-facto rule) to use - instead of = to separate the prefix of your CSS class.

In case of bad HTML, browsers typically try to fix the error by themselves. A = is part of the HTML tag syntax and may be badly interpreted as an attribute in case a correction is needed. Of course we aim at providing valid HTML to the client, but those things can still happen.

anthonygego · 2023-07-07T10:14:46Z

inginious/frontend/static/js/common.js

                    mode = nmode;
+                } else if (elem.startsWith('theme=')) {


... continuation of my comment about -and =.

anthonygego · 2023-07-07T10:17:32Z

inginious/frontend/user_manager.py

@@ -33,6 +33,14 @@ class AuthInvalidMethodException(Exception):

 class AuthMethod(object, metaclass=ABCMeta):

+    @abstractmethod


Those methods do not have to be defined in AuthMethod class as there should be no dependance between them.

anthonygego · 2023-07-07T10:18:25Z

inginious/frontend/user_manager.py

@@ -107,6 +115,12 @@ def sanitize_email(cls, email: str) -> str:
    #           User session management          #
    ##############################################

+    def get_theme(self):


I'd rather call them session_theme and session_codemirror_theme to use the same nomenclature and recall throughout the code that those values are fetched from the user session.

anthonygego · 2023-07-07T10:19:21Z

inginious/frontend/templates/tasks/code.html

@@ -7,6 +7,7 @@

 <textarea name="{{inputId}}"
          class="code-editor form-control {% if '/' in inputId %} single {% endif %}"
+          theme="dracula"


Hardcoded value :-)

anthonygego · 2023-07-07T10:25:40Z

inginious/frontend/themes/Gruntfile.js

@@ -0,0 +1,170 @@
+'use strict';


You can rename themes/themes into themes/src to be less confusing.

You should probably add an initial comment such as

Based on Bootswatch from Thomas Park (https://github.com/thomaspark/bootswatch)

in the beginning of the imported files

…ation

… admin

Updated setup requirements to Flask 2.0+ that introduces the new function. Fix UCL-INGI#942

Ananas120 · 2023-09-25T07:15:18Z

I still update this branch PR to be sure, I will now try to create a new one on the tasksets branch...

Ananas120 · 2023-09-25T07:52:58Z

All the commands you told me have (in theory) been successfully executed, and conflicts solved ! Hope it is correct now
Note that for the "theme=" comment, I have changed to "theme:" instead of "theme-" because some codemirror themes have the "-" symbol, which may raise errors when splitting it

new bootswatch theme compilation

cce463f

anthonygego requested changes Apr 6, 2023

View reviewed changes

Merge branch 'UCL-INGI:master' into master

2b535fc

update PR themes modification

252ccaa

anthonygego requested changes May 4, 2023

View reviewed changes

Ananas120 and others added 4 commits May 10, 2023 08:10

Merge branch 'UCL-INGI:master' into master

6e58302

update PR + new codemirror themes

9ff71ff

Merge branch 'UCL-INGI:master' into master

aad5c64

Merge branch 'master' of https://github.com/Ananas120/INGInious

d7831c9

anthonygego requested changes Jul 7, 2023

View reviewed changes

anthonygego added 17 commits August 8, 2023 17:15

[course_admin/task_list] add support for automated task settings migr…

fa24e8a

…ation

[frontend/task_dispensers] fix tooltip issue in task buttons

4f40a72

[task_dispensers] grouped actions : setting selection and preview

3cc8327

[task_dispensers] move setting filter bar to util.html

b8bcd6e

[task_dispensers] grouped actions: allow for group selections

7408cb6

[task_dispensers] allow settings change for group of tasks

0f33120

[task_dispensers] allow grouped task deletion

1a1901c

[task_dispensers] allow to move multiple tasks to one section

98212de

[task_dispensers] apply rendering for new and existing tasks

e53e6c4

[task_dispensers] allow dispenser settings for new tasks

0991e81

[task_dispensers] enhance error handling in task list

d05168a

[task_dispensers] enable compact view by default

1cca275

[task_dispensers] remove categories specific code from main code

c42d44f

Update minified js

fa501dd

[tasksets] rename course to taskset in the backend files

408bc78

[tasksets] courses built on top of tasksets

d37d234

[tasksets] update tests

93e301d

anthonygego and others added 17 commits August 22, 2023 13:10

[courses] remove taskset edition capability from courses

0b206d6

[courses] merge admins and tutors at the course level

d601628

[task_dispensers] allow course templating from tasksets

48ebcec

[task_dispensers] improve performance by reducing html/js code

b6366da

Update minified js

5472789

[task_dispenser/toc] init config dict if empty

94e47ff

[courses/user_settings] clearly distinguish user settings from course…

6826dbd

… admin

[courses] distinguish courses from tasksets using fa icons

f53b98f

[i18n] fix jinja2 imports for Jinja3+

dedbdbd

[i18n] update french translations

fb5adb1

update PR before rebase

218facb

[frontend/flask] fix session_cookie_name deprecation

b15462c

Updated setup requirements to Flask 2.0+ that introduces the new function. Fix UCL-INGI#942

[setup] temporarily restrict to requests<2.2.29

c2ce8c8

[frontend/themes] [1/2] main files update

9a45e31

[frontend/themes] [2/2] second theme files update

70691bc

Update demo tasks repo URL to new organisation (UCL-INGI#947)

9202efd

update PR before rebase

595680e

Ananas120 added 6 commits September 25, 2023 09:16

Merge branch 'master' of https://github.com/Ananas120/INGInious

e771838

new bootswatch theme compilation

244004d

update PR themes modification

d879278

[frontend/themes] [1/2] main files update

fde4fac

[frontend/themes] [2/2] second theme files update

0533452

update PR before rebase

3f5a7bb

Ananas120 added 3 commits September 25, 2023 09:55

update PR + new codemirror themes

094de20

update PR before rebase

8e42845

Merge branch 'master' of https://github.com/Ananas120/INGInious

567d98f

nrybowski mentioned this pull request Oct 24, 2023

[frontend/themes] new black theme + codemirror themes #975

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[frontend/theme] script for bootswatch theme compilation #931

[frontend/theme] script for bootswatch theme compilation #931

Ananas120 commented Apr 3, 2023

CLAassistant commented Apr 3, 2023 •

edited

Loading

anthonygego left a comment

anthonygego Apr 6, 2023

anthonygego Apr 6, 2023

anthonygego Apr 6, 2023

anthonygego Apr 6, 2023

Ananas120 commented Apr 26, 2023

anthonygego left a comment

anthonygego May 4, 2023

anthonygego May 4, 2023

anthonygego May 4, 2023

anthonygego May 4, 2023

anthonygego May 4, 2023

anthonygego May 4, 2023

anthonygego May 4, 2023

anthonygego May 4, 2023

anthonygego May 4, 2023

anthonygego May 4, 2023

Ananas120 commented Jun 1, 2023

anthonygego left a comment

anthonygego Jul 7, 2023

anthonygego Jul 7, 2023

anthonygego Jul 7, 2023

anthonygego Jul 7, 2023

anthonygego Jul 7, 2023

anthonygego Jul 7, 2023

anthonygego Jul 7, 2023

anthonygego Jul 7, 2023

anthonygego Jul 7, 2023

Ananas120 commented Sep 25, 2023

Ananas120 commented Sep 25, 2023

		@@ -33,6 +33,14 @@ class AuthInvalidMethodException(Exception):

		class AuthMethod(object, metaclass=ABCMeta):

		@abstractmethod

[frontend/theme] script for bootswatch theme compilation #931

Are you sure you want to change the base?

[frontend/theme] script for bootswatch theme compilation #931

Conversation

Ananas120 commented Apr 3, 2023

CLAassistant commented Apr 3, 2023 • edited Loading

anthonygego left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ananas120 commented Apr 26, 2023

anthonygego left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ananas120 commented Jun 1, 2023

anthonygego left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ananas120 commented Sep 25, 2023

Ananas120 commented Sep 25, 2023

CLAassistant commented Apr 3, 2023 •

edited

Loading