refactor (various) to move inline styles

[koeaw]

Move leftover inline styles from HTML templates to static CSS files.

Closes #1258

---

Not included are the scroll button styles already covered by #1234 and styling applied in Python code as that's harder to untangle. Ex.

apis-core-rdf/apis_core/apis_relations/forms.py

Lines 49 to 54 in cda3436

	attrs = {
	"data-placeholder": "Type to get suggestions",
	"data-minimum-input-length": getattr(settings, "APIS_MIN_CHAR", 3),
	"data-html": True,
	"style": "width: 100%",
	}

The styles in apis_relations.css may be redundant as they belong to relations_detail_generic.html, see #1259, but I didn't want to leave strays. I've added TODO comments in the file to make note of that.

[b1rger]

I'm removing myself from the reviewers as I won't have time to look at this.

There is only one commit that touches multiple apps, it could easily be split up into smaller, easier to review, commits. It also mixes the refactoring with functional changes, i.e. replacing the style assignment in one element with a css class in another element without any explanation as to why. It also moves bootstrap inclusion without any mention in the commit message and overrides bootstrap code without explanation except the comment "fixes an issue with select2".

Feel free to assign to someone who either has more time to untangle all that stuff or who knows what all those changes mean by simply looking at them

[koeaw]

I'm removing myself from the reviewers as I won't have time to look at this.

Ok.

There is only one commit that touches multiple apps, it could easily be split up into smaller, easier to review, commits.

I didn't think of splitting the changes up because they didn't seem substantial to me, but point taken about the scoping.

It also mixes the refactoring with functional changes, i.e. replacing the style assignment in one element with a css class in another element without any explanation as to why.

I understand refactoring to mean to replace [code] (markup, whatever) with other (simpler, cleaner etc.) [code] to achieve the same result. That's what these changes are doing.

Moving "pointers" to elements to other elements in the chain and manipulating them from there is a basic concept in CSS, so it didn't even occur to me it may need explaining. But I'm coming at this from a full-stack perspective, and frontend development is a whole separate field of work with its own separate domain-specific knowledge and skill sets. But frontend stuff is not a primary concern in this project. So it's difficult to know what or how much detail to try to cram into e.g. commit messages.

I'd suggest we discuss how we should deal with frontend-related code & documentation at a JF.

It also moves bootstrap inclusion without any mention in the commit message and overrides bootstrap code without explanation except the comment "fixes an issue with select2".

Ah, thx for pointing out the HTML move got subsumed here.

I refactored the template away because it seemed pointless to keep for the one remaining line of HTML (+ the comment) after moving the styles, but you are right, that shouldn't have got included.

[b1rger]

There is only one commit that touches multiple apps, it could easily be split up into smaller, easier to review, commits.

I didn't think of splitting the changes up because they didn't seem substantial to me, but point taken about the scoping.

Its not primarily about the scope, splitting up into commits per app was only an example. Its about consideration for the person that has to review the PR. Also, if there are 5 different changes in a commit and one of those changes require more discussion, the remaining 4 changes have to be postponed until the discussion points are resolved. Some of those changes in the PR at hand could already have been merged.

Regarding the commit messages, I don't think that full-stack/frontend changes should be held to a lower standards than other changes. From https://www.freecodecamp.org/news/how-to-write-better-git-commit-messages/

To come up with thoughtful commits, consider the following:
Why have I made these changes?
What effect have my changes made?
Why was the change needed?
What are the changes in reference to?

See also this example: https://dhwthompson.com/2019/my-favourite-git-commit

Regarding the specific wording:
"move inline styles" sounds to me as if the inline styles where moved somewhere else, which is not what is happening in those commits. The inline styles are replaced by non-inline styles.

[koeaw]

Regarding the specific wording: "move inline styles" sounds to me as if the inline styles where moved somewhere else, which is not what is happening in those commits. The inline styles are replaced by non-inline styles.

IMO the wording describes well enough what's happening here.

"Inline styles" means "styles which are applied (or which appear/...) on a line with HTML". When "these styles which appear inline" are put in an external file, that's primarily a matter of placement, not replacement. I think it's perfectly legitimate to call this "moving".

This is no different from moving Django template code from one template file (block,...) to another. Yes, sure, if I wanted to be literal, I could say I'm replacing x lines of template language with an include statement + a new file with a duplicate of those x lines, but I didn't think I would be misunderstood if I called this moving those lines to a separate template/include/...?

Would it help if the commit subjects were changed to e.g. "move styles to external files"?

[b1rger]

IMO the wording describes well enough what's happening here.

🤷
suit yourself

[gythaogg]

Do we need to scope the style to child elements of an element with id="modal"? I am assuming that it's fair for all elements with modal-dialog in their classes to pick up this style...

[koeaw]

I don't understand the customisation in the first place. Out of the box, the component looks like what I'd expect a modal to typically look like. This seems to hard-override a max-width setting originally defined via @media rules to create a wide-screen version of that.

I would not force this formatting on any and all components of this type. Modals are primarily meant for dialogues, which I expect to contain little text, which shouldn't be made harder to read by increasing paragraph width.

[koeaw]

In a next step, I'd suggest to replace the custom styling with the modifier class Bootstrap provides. But that's for another PR.

[koeaw]

Ok, I just checked – the modifier actually results in an equivalent largest width. I can switch it out if you don't mind this turning the commit into only a half refactor/half feature (added smaller container width at smaller screen width).

[koeaw]

... or, rather, a fix. (;

[koeaw]

@gythaogg Any more feedback on this?

[gythaogg]

@koeaw the code changes look fine to me but I couldn't figure out how to test all of the changes; since I've been burnt before by styles losing precedence once they were moved away from inline I want to just do a sanity check.

Is it okay if we sit together so I can use your help to see that the all the moved styles are picked up?

[gythaogg]

Verified the places (except apis_relations) where the new styles replace inline styles and there is no difference - all the styles are getting applied as they should.

[koeaw]

Thank youuu!