search

equinor / webviz-ert

ERT webviz plugins
GNU General Public License v3.0
12 stars 24 forks source link

Use namespaced class for selectors #392

Closed valentin-krasontovitsch closed 2 years ago

valentin-krasontovitsch commented 2 years ago

Due to webviz build setup, and the way we bundle software at equinor (komodo), class names in our assets can affect other webviz apps which do not refer to our codebase in any way.

We should always use namespaced class names for modifying CSS properties rather then the first existing name we find.

Approach Use ert-select-variable instead of webviz-config-select

Pre review checklist

valentin-krasontovitsch commented 2 years ago

This was kindly pointed out by Ruben @rubenthoms after receiving a hint from a user, and hopefully not debugging for way too long before finding this :grimacing:

in the future, webviz shall be more restrictive with what gets included from existing (installed) plugins. also, the guidelines / best practices will be updated to hint on the fix in the meantime, namely using namespaced class names, or some other solution (like defining your own class inheriting from core components with specific style set in python code, circumventing the need to define style in assets).

oysteoh commented 2 years ago

Jenkins: Test this please!

valentin-krasontovitsch commented 2 years ago

jenkins failed due to a seemingly flaky test in spe1 - 

=================================== FAILURES ===================================
_______________________ test_webviz_response_correlation _______________________

>       dash_duo.wait_for_text_to_equal(selector_id, expected_text)

tests/test_webviz_ert.py:96:

see jenkins logs for moar details[INTERNAL LINK]

this is something that dan has been working on. i will restart the jenkins for now, just to see if it will pass (whether the test really is flaky or perhaps breaks for other reasons, like a wrong index).

valentin-krasontovitsch commented 2 years ago

jenkins, retest this please!

DanSava commented 2 years ago

jenkins, retest this please!

valentin-krasontovitsch commented 2 years ago

well, jenkins passed on this PR, not sure if we want to merge or wait until the failing tests in bleeding etc. are resolved...

valentin-krasontovitsch commented 2 years ago

jenkins, retest this please!

oysteoh commented 2 years ago

Jenkins: Test this please!

oysteoh commented 2 years ago

Jenkins: Test this please!