Do not set style directly in structural-subscriptions.js

Bug #752406 reported by Данило Шеган
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Launchpad itself
Triaged
High
Unassigned

Bug Description

In lib/lp/registry/javascript/structural-subscriptions.js, we set style directly in a bunch of places. They should all be fixed to instead set/unset CSS classes, and CSS classes should be defined outside the JS file.

tags: added: story-better-bug-notification
tags: added: tech-debt
Changed in launchpad:
status: New → Triaged
importance: Undecided → Medium
importance: Medium → High
Revision history for this message
Gary Poster (gary) wrote :

I wonder about the importance/wisdom of this policy in one-location-JS-app code like this. The class will not be shared; and the application of the style locally follows DRY because of how we do it; and IMO making it as a CSS class in the shared launchpad CSS will make it harder, not easier, to refactor the JS.

Because of this, and because a "high" bug blocks us from declaring that we are done with our LEP, I'm reducing this to low. I'm tempted to even mark this as Won't Fix and see who argues back, but I suspect this comment will already accomplish the purpose of getting dissenting or agreeing replies.

Changed in launchpad:
importance: High → Low
Gary Poster (gary)
Changed in launchpad:
importance: Low → Medium
Revision history for this message
Gary Poster (gary) wrote :

We discussed this on IRC. Apparently there is some disagreement about this across LP devs right now. For our squad, we decided on this resolution (from Danilo): we will add our own CSS file and establish a practice for that (bin/combine_css has a simple names=[] variable where we can manually list it for now). That gives us the ability to see what is going on more easily for things that are specific to our code, which is one of my main concerns; while letting us use CSS classes, which many regard as cleaner for all use cases (or at least more use cases that we are using them for now).

Revision history for this message
Graham Binns (gmb) wrote :

As I remarked to Gary on IRC just now, we need to make some further considerations before going ahead with this, if at all:

 - Should we have CSS classes for styles that are very element-specific?
 - Should we also replace setStyle() calls with addClass() and removeClass() calls?

(These are just the first two that occurred to me; I'm sure there are more).

Changed in launchpad:
importance: Medium → High
tags: added: javascript
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.