[colors] Add a "auto" color #596

olivierphi · 2022-06-29T13:59:48Z

When text has color: auto [percentage] it will be displayed with the contrasting colour of its background

As a result we can have widgets that have color: auto; in their CSS, and see their text colour automatically adapted to their background:

When text has "color: auto [percentage]" it will be displayed with the contrasting color of its background

olivierphi · 2022-06-29T14:01:23Z

src/textual/color.py

+    @property
+    def is_auto(self) -> bool:
+        """Check if the color is "auto"."""
+        return self.r == AUTO.r and self.g == AUTO.g and self.b == AUTO.b


Would probably be more elegant with (self.r, self.g, self.b) == (AUTO.r, AUTO.g, AUTO.b), but since we're aiming at optimising for performance in most cases I guess creating a tuple takes a bit more time than doing this triple equality condition? 🙂

self[:3] == AUTO[:3] is probably fastest. But I'm afraid I'm going to veto this sentinel color object.

olivierphi · 2022-06-29T14:02:16Z

sandbox/color_names.py

@@ -35,8 +34,7 @@ def on_mount(self):
    def compose(self) -> ComposeResult:
        for color_name, color in COLOR_NAME_TO_RGB.items():
            color_placeholder = ColorDisplay(name=color_name)
-            is_dark_color = sum(color) < 400
-            color_placeholder.styles.color = "white" if is_dark_color else "black"
+            color_placeholder.styles.color = "auto 90%"


This "is_dark_color" test is no longer required, since the new auto property does exactly this 😊

The result:

darrenburns

Looks good to me :)

willmcgugan

I think we need an alternative way of specifying the "auto" foreground color.

It may not be necessary to modify Color at all. When parsing, we could set a bool on the Styles object such as "auto_color". I think this would capture the requirement that only color can be auto.

Please try to minimize the use of integration tests right now. I would like to aim to have as much coverage as possible with unit tests.

willmcgugan · 2022-06-29T14:17:38Z

src/textual/color.py

@@ -63,7 +63,8 @@ class Lab(NamedTuple):
 rgb{OPEN_BRACE}({DECIMAL}{COMMA}{DECIMAL}{COMMA}{DECIMAL}){CLOSE_BRACE}$|
 rgba{OPEN_BRACE}({DECIMAL}{COMMA}{DECIMAL}{COMMA}{DECIMAL}{COMMA}{DECIMAL}){CLOSE_BRACE}$|
 hsl{OPEN_BRACE}({DECIMAL}{COMMA}{PERCENT}{COMMA}{PERCENT}){CLOSE_BRACE}$|
-hsla{OPEN_BRACE}({DECIMAL}{COMMA}{PERCENT}{COMMA}{PERCENT}{COMMA}{DECIMAL}){CLOSE_BRACE}$
+hsla{OPEN_BRACE}({DECIMAL}{COMMA}{PERCENT}{COMMA}{PERCENT}{COMMA}{DECIMAL}){CLOSE_BRACE}$|
+(auto)(?:\s+({DECIMAL})%)?$


I'm not sure we need the decimal percentage here. We can already apply alpha with a color and a percentage.

We also don't want to enforce an order. i.e. "50% red" should be the same as "red 50%"

willmcgugan · 2022-06-29T14:20:08Z

src/textual/color.py

+    @property
+    def is_auto(self) -> bool:
+        """Check if the color is "auto"."""
+        return self.r == AUTO.r and self.g == AUTO.g and self.b == AUTO.b


self[:3] == AUTO[:3] is probably fastest. But I'm afraid I'm going to veto this sentinel color object.

willmcgugan · 2022-06-29T14:37:03Z

tests/test_integration_color.py

@@ -0,0 +1,77 @@
+import pytest


These tests run a lot of code that is unrelated to the PR.

I'd like to focus on unit tests, for instance:

Test a color can be marked as auto

Test Color.parse("auto") produces an auto color

Test setting background and color on a Styles object, produces the expected Styles.rich_style

Test parsing the CSS produces the expected rules

None of the above require a running app or async context. They are less likely to break from an unrelated API change.

With only unit tests how do we check that the "auto" takes into account the colours and opacity levels of all the Node's ancestors?

That is a good point. I think you can create a hierarchy without running the app. Create the App, call app.mount.

That's what I was thinking too initially, and that's the first path I tried to take for such cases back in the days. But it's too complicated without a real running App as we need the compositor and that kind of things to do their magic, and some of those classes rely on the presence of a non-null active_app in "textual._context" 😬 - which is why I ended up creating that integration test framework for Textual.

I guess we can keep that test now that it's there, as it successfully tests more advanced scenarios than just an element checking its own "auto" colour with its own background colour?
And unless I'm missing something, if we introduce an API change I reckon we'll just have to update the integration tests accordingly, like we do for unit tests?

Yeah, keep this integration test, since it's done. But let's add some unit test for what we can easily test. We may have to solve a few such issues a active_app to make unit tests more practical in the future.

Integration tests are more likely to break with API changes. And the breakage is unlikely to tell me anything I didn't know already.

Integration tests will come in to their own when the APIs are more stable. They can catch unintended API changes rather than intentional API changes.

olivierphi · 2022-06-30T16:03:24Z

tests/css/test_parse.py

+            ("hsla(180,50%,50%,0.25)", Color(64, 191, 191, 0.25)),
+            ("hsla( 180, 50% ,50%,0.25 )", Color(64, 191, 191, 0.25)),
+            ("hsla( 180, 50% , 50% , 1.5 )", Color(64, 191, 191)),
+            ("red 50%", Color(255, 0, 0, 0.5)),


I just added these 2 cases, and Black did the rest 😄

olivierphi · 2022-06-30T16:04:24Z

tests/test_integration_color.py

+        ["30% auto", "black", BLACK.get_contrast_text(0.3)],
+    ),
+)
+def test_color_auto_single_widget(


here is a "closer to the metal" test for the "auto" colour, that can only test that a Widget adapts its "auto" colour to its own background :-)

olivierphi · 2022-06-30T16:10:24Z

@willmcgugan the implementation is twice more verbose than the "sentinel 'auto' Color" one, and I'm not a big fan of the fact that we now offload a characteristic of a "color" property to another property of the stylesheet. The whole thing is more complicated now imho.
But here we go... This new implementation works now, with the tests of the previous one still passing as is 🙂

willmcgugan · 2022-06-30T19:02:55Z

More complicated maybe, but I think this a better abstraction. If you consider that an object should have one reason to exist, a Color class should only ever abstract a color (and "auto" is not a color). "auto" to my mind is more of a way of enabling functionality on the Style object.

willmcgugan

I like the update, but I think we need one more iteration on parsing the colors.

willmcgugan · 2022-06-30T19:18:50Z

src/textual/css/_color.py

@@ -0,0 +1,57 @@
+"""


We're doing some custom parsing here, which is different from the parsing done in the CSS. I'd like to avoid solving the same problem twice.

We could use tokenize_value in textual.css.tokenize for this -- which is what the CSS parser uses.

Here's an example:

>>> from textual.css.tokenize import tokenize_value >>> list(tokenize_value("hsl( 180, 50%, 50% ) 30%", "expression")) [Token(name='color', value='hsl( 180, 50%, 50% )', path='expression', code='hsl( 180, 50%, 50% ) 30%', location=(0, 0)), Token(name='whitespace', value=' ', path='expression', code='hsl( 180, 50%, 50% ) 30%', location=(0, 20)), Token(name='scalar', value='30%', path='expression', code='hsl( 180, 50%, 50% ) 30%', location=(0, 21))]

This should return the same tokens passed to the builder object.

How about the property uses tokenize and calls a method to extract a Color from the tokens. The builder could also call this same method.

My reasoning was that simply doing a join over the tokens in the StylesBuilder was much faster than tokenizing the values in the ColorProperty - and the 2 regexes used to find the percentage values likely have a fast lookup since they only search at the beginning and the end of a string.
But ok, I'll do that 🙂

src/textual/css/_style_properties.py

willmcgugan · 2022-06-30T19:24:04Z

src/textual/css/_styles_builder.py

-                    )
-            else:
-                self.error(name, token, color_property_help_text(name, context="css"))
+        expression = _join_tokens(tokens)


We're parsing the color, combining the tokens, and re-parsing the string again. See my suggestion re tokenize above. I think we can simplify by extracting the color from a list of tokens.

willmcgugan · 2022-06-30T19:25:10Z

src/textual/css/styles.py

@@ -182,6 +183,9 @@ class StylesBase(ABC):
    layout = LayoutProperty()

    color = ColorProperty(Color(255, 255, 255))
+    # N.B. This one is a pseudo-property, in the sense that we cannot set it via CSS via a dedicated property:


We can also set it via code can't we? self.styles.color_auto = True ?

We can yes, what my comment was meaning is that we cannot set it via a CSS string or file - as there is no process_color_auto method in the StylesBuilder.
That's why to me it's a pseudo-property, as it can only be set via another CSS property, "color" 🙂

willmcgugan · 2022-06-30T19:27:46Z

src/textual/css/_color.py

+)
+
+
+def parse_color_expression(color_text: str) -> tuple[Color, bool]:


As a pure function this is easy to test. Can we have some unit tests?

…lpha" part Also add a unit test for this new `parse_color_tokens` function

olivierphi · 2022-07-01T09:52:25Z

tests/test_color.py

-    ("hsl( 45, 25% , 25% )", Color(80, 72, 48)),
-    ("hsla( 45, 25% , 25%, 0.35 )", Color(80, 72, 48, 0.35)),
-])
+@pytest.mark.parametrize(


just Black ⚫ doing its things, I haven't updated this test 🙂

olivierphi · 2022-07-01T09:52:37Z

tests/test_color.py

-    ("hsl(400, 200%, 250%)", Color(255, 255, 255, 1.0)),
-    ("hsla(400, 200%, 250%, 1.9)", Color(255, 255, 255, 1.0)),
-])
+@pytest.mark.parametrize(


same here ⚫

olivierphi · 2022-07-01T09:52:46Z

tests/test_color.py

@@ -141,7 +151,8 @@ def test_color_parse_hsl_negative_degrees():

 def test_color_parse_hsla_negative_degrees():
    assert Color.parse("hsla(-45, 50%, 50%, 0.2)") == Color.parse(
-        "hsla(315, 50%, 50%, 0.2)")
+        "hsla(315, 50%, 50%, 0.2)"


olivierphi · 2022-07-01T09:53:03Z

tests/test_color.py

@@ -219,3 +230,24 @@ def test_rgb_lab_rgb_roundtrip():
 def test_color_pair_style():
    pair = ColorPair(Color(220, 220, 220), Color(10, 20, 30))
    assert str(pair.style) == "#dcdcdc on #0a141e"
+
+
+@pytest.mark.parametrize(


I did add this test though 😄

olivierphi · 2022-07-01T09:56:31Z

@willmcgugan Feedback (hopefully) addressed! 🙂 28edacf

willmcgugan

Consider this...

self.styles.color = "red"

Fantastic, we have red text. I now do this:

self.styles.color = "auto 90%"

Great, the text is now white-ish or black-ish. All good so far.

self.styles.color_auto = False

Now what color is the text? I suspect it will be Color(1,2,3, a=0.9) which would be a surprise to the developer.

My gut feeling is that it should be red again.

I think we should solve this. Could you have a think about a solution and let's have a chat about it when you have a plan?

willmcgugan · 2022-07-01T11:31:08Z

src/textual/css/_color.py

+    is_auto = False
+
+    for token in tokens:
+        if token.value == "auto":


It's not enough to look at just the value. We also need to change "name" as well, which I think should be "token".

Consider if we add quoted strings to the parser, we wouldn't want to interpret "auto" in the same way as auto

willmcgugan · 2022-07-01T11:35:27Z

src/textual/css/styles.py

-        if has_rule("color"):
+        if get_rule("color_auto", False):
+            append_declaration(
+                "color", f"auto{f' {self.color.a * 100}%' if self.color.a < 1 else ''}"


Can we do this without nested f strings? I find that hard to parse.

willmcgugan · 2022-07-01T11:38:22Z

tests/test_color.py

+        ["32% auto", Color(ANY, ANY, ANY, 0.32), True],
+    ),
+)
+def test_parse_color_expression(


Can we have one or two invalid color strings tests? i.e. the case where it fails.

willmcgugan · 2022-07-01T11:42:59Z

src/textual/css/_style_properties.py

            try:
-                parsed_color = Color.parse(color)
+                parsed_color, is_auto = parse_color_tokens(self.name, tokens)


This can raise DeclarationErrors can't it?

This would be entirely unexpected in this context. It should raise StyleValueErrors

willmcgugan · 2022-07-01T11:51:53Z

src/textual/css/styles.py

-        if has_rule("color"):
+        if get_rule("color_auto", False):
+            append_declaration(
+                "color", f"auto{f' {self.color.a * 100}%' if self.color.a < 1 else ''}"


Suggested change

"color", f"auto{f' {self.color.a * 100}%' if self.color.a < 1 else ''}"

"color", f"auto {self.color.a * 100}%" if self.color.a < 1 else "auto"

darrenburns · 2022-07-01T12:38:57Z

I have to say I'm really not a fan of the color_auto - it's so tightly coupled to color and having 2 properties interfering with each other like that is just confusing. If we have to guess what the behaviour is for something that is conceptually so simple, I think we're already going to the wrong route.

I liked the idea of a sentinel value but agree the previous approach broke the "color" abstraction.

IMO it'd be nice to have the color property value have a type Color | Auto (i.e. a union where Auto is some container or sentinel to mark the special case), rather than having a separate flag. The approach as it stands feels like "using a flag to simulate a union type".

willmcgugan · 2022-07-01T13:06:31Z

So self.styles.color could return a Color or an Auto?

What kind of interface would the Auto support? Can I write self.styles.color.r or would I have to do is isinstance(self.styes.color) beforehand?

olivierphi · 2022-07-01T13:30:54Z

So self.styles.color could return a Color or an Auto?
What kind of interface would the Auto support? Can I write self.styles.color.r or would I have to do is isinstance(self.styes.color) beforehand?

Maybe the problem is that we're trying to fit something that is not a colour (and that doesn't exists in "browser's CSS", so we cannot take inspiration from there) into a property named "color"? 🤔

Shouldn't we rather follow the "principle of least surprise", and say that if it's definitely not a colour it shouldn't be set via a property that has this name?
Maybe it could go under something like a standalone color-auto property, which value could either be:

set (I guess it could be a synonym for the value 95%, which is the default value for the get_contrast_text method?)
unset (similarly to border: unset for example in browsers' CSS, so one can cancel the property if it's been set somewhere else)
an alpha? (in either percentage or fractional units?)

willmcgugan · 2022-07-01T13:40:18Z

I did not mean to do that!

olivierphi · 2022-07-01T13:47:54Z

While we're at it I'm putting this here - mostly for myself, for future reference 😄

List of the "universal" values we can apply to any CSS property in browsers' CSS:

initial: to set a property to its initial value.
inherit: to make an element's property the same as its parent.
unset: to set a property to its inherited value if it inherits or to its initial value if not - combination of the initial and inherit keywords
revert: to reset a property to the value established by the user-agent stylesheet (or by user styles, if any exist).
revert-layer: to reset a property to the value established in a previous cascade layer.

willmcgugan · 2022-07-01T13:55:50Z

I agree that styles.color should probably not be anything other than a Color. It would surprise me if that wasn't the case..

For the Python interface, I think we will need color_auto as a boolean, but also color_auto_alpha as a float to capture the alpha color. I think that would avoid the edge case I mention above.

@drbenton I see where you are going with another CSS property to capture the auto color state, but find the declaration of "color: auto 90%" to be quite elegant. And another rule might confuse, especially if there is a "color" and a "color-auto".

olivierphi · 2022-07-01T14:07:41Z

For the Python interface, I think we will need color_auto as a boolean, but also color_auto_alpha as a float to capture the alpha color. I think that would avoid the edge case I mention above.

Alright, let's do this! *cracks his knuckles

willmcgugan · 2022-07-01T14:27:50Z

Just for the record. After a 3 way call I think we've agreed on the following:

The Styles.color property setter can accept "auto" with an optional alpha, e.g. "auto 90%"
The Styles.color property getter returns the calculated contrasting color.
We add a Styles.color_auto boolean (getter only) that exposes if "auto" is in effect.

I think this alleviates any edge cases, and is reasonably easy to wrap ones head around.

Thumbs up if you're in favor...

…se rather than a CSS property This solves the issue of having both a genuine color defined and a "color: auto", and unsetting the `color_auto` CSS property (which would leave us with the pseudo-Color we were creating to hold the value of the "auto" alpha)

olivierphi · 2022-07-01T15:32:54Z

As I was implementing this I realised that there is a slight issue 😅 :

In order to make ColorProperty return a conytrasting colour on the fly when we request its value, like we said, we would need it to compute that value by traversing the Node's ancestors tree.
The problem with this is that we already do exactly this in the DOMNode's colors getter, which not only returns this colour but also the base_background, base_color and background ones.

--> so we would do a calculation of the colour that needs to traverse all the Node's ancestors in this colors getter, but if there is a color: auto in the mix each ancestor would also calculate its colour the fly using its own ancestors in the ColorProperty __get__ method- which gives us a loop-over-ancestors wrapped in another loop-over-ancestors 😬

However, I realised that we can solve the original issue (being able to do widget.styles.color_auto = False) pretty simply, by following this "private property / public getter" principle we were mentioning: if the implementation is still the same but _color_auto is now just a private property of the StylesBase class, the issue is actually solved! ✔️ 😄

--> 94ba806

willmcgugan · 2022-07-01T16:06:21Z

@drbenton I'm not sure I follow. Let's have a chat about it on Tuesday.

…rty descriptor

olivierphi · 2022-07-05T13:51:11Z

@willmcgugan here you go! 4439ca1

willmcgugan · 2022-07-05T14:42:07Z

src/textual/css/_style_properties.py

+        # calculate what the resulting value should be.
+        from ..dom import DOMNode
+
+        styles_node = getattr(obj, "node", None)


Not nice, but I see the problem. I wonder if node should be set to None on StyleBase so we can guarantee the attribute is present.

willmcgugan · 2022-07-05T14:43:35Z

src/textual/css/_style_properties.py

+            raise ValueError(
+                "Dynamic 'auto' color can only be used with an subclass of StylesBase that has a 'node' attribute"
+            )
+        # We just repeat what `DOMNode.color` does, in a slightly simplified way.


Have you considered rolling this logic in to the color property itself?

The thing is that DOMNode.colors returns 4 computed colours - foreground and background colours, and its parent's colours. Whereas the Style properties each manage their single property

willmcgugan · 2022-07-05T14:50:50Z

src/textual/css/_style_properties.py

        Args:
            obj (Styles): The ``Styles`` object.
            objtype (type[Styles]): The ``Styles`` class.

        Returns:
            Color: The Color
        """
-        return cast(Color, obj.get_rule(self.name, self._default_color))
+        if self.name != "color" or not obj.color_auto:


Rather than overload the functionality of this object based on its name. We could define a property specifically for the foreground color.

This would make it easier to keep the functionality distinct.

I think this may already be an issue. For instance, if you do styles.background = "auto 90%" I'm guessing that would set the foreground to auto? Which would not be desirable.

…perty descriptor

olivierphi · 2022-07-06T09:32:00Z

src/textual/css/_style_properties.py

@@ -799,6 +799,8 @@ def __set__(self, obj: StylesBase, names: str | tuple[str] | None = None):
 class ColorProperty:
    """Descriptor for getting and setting color properties."""

+    _supports_auto: ClassVar = False


I could do without this class var, but if I do so there will be quite a lot of code duplication between the __get__ methods of the ColorProperty and the new ForegroundColorProperty.
As always, it's a trade-off 😅

olivierphi · 2022-07-06T09:32:37Z

src/textual/css/_style_properties.py

@@ -891,13 +860,74 @@ def __set__(self, obj: StylesBase, color: Color | str | None):
                )

            if is_auto:
-                obj._color_auto = True
+                if not self._supports_auto:
+                    raise StyleValueError(


added a test to check that this does its job 👇

olivierphi · 2022-07-06T09:33:10Z

src/textual/css/_style_properties.py

+        # N.B. `DOMNode.color` will call this `__get__` method while it is itself traversing the DOMNode ancestors,
+        # so we loop over ancestors in a loop that is itself looping over ancestors.
+        # That's a deliberate implementation choice.
+        background = Color(0, 0, 0, 0)


this dynamic resolution is now only made for the "color" property

[colors] Add a "auto" color

1cf8a5e

When text has "color: auto [percentage]" it will be displayed with the contrasting color of its background

olivierphi commented Jun 29, 2022

View reviewed changes

olivierphi marked this pull request as ready for review June 29, 2022 14:04

olivierphi requested review from darrenburns and willmcgugan June 29, 2022 14:04

darrenburns approved these changes Jun 29, 2022

View reviewed changes

willmcgugan requested changes Jun 29, 2022

View reviewed changes

olivierphi force-pushed the implement-color-auto branch from 61fd2f3 to fcbb033 Compare June 30, 2022 15:57

olivierphi commented Jun 30, 2022

View reviewed changes

[colors] Move the management of "auto" color to the Style class

b9662c7

olivierphi force-pushed the implement-color-auto branch from fcbb033 to b9662c7 Compare June 30, 2022 16:04

olivierphi requested a review from willmcgugan June 30, 2022 16:10

willmcgugan requested changes Jun 30, 2022

View reviewed changes

[colors] Use tokens rather than regex to handle color expressions' "a…

28edacf

…lpha" part Also add a unit test for this new `parse_color_tokens` function

olivierphi commented Jul 1, 2022

View reviewed changes

olivierphi requested a review from willmcgugan July 1, 2022 09:56

willmcgugan requested changes Jul 1, 2022

View reviewed changes

willmcgugan closed this Jul 1, 2022

willmcgugan reopened this Jul 1, 2022

olivierphi requested a review from willmcgugan July 1, 2022 15:33

[colors] "color_auto" are now calculated on the fly by the ColorPrope…

4439ca1

…rty descriptor

willmcgugan reviewed Jul 5, 2022

View reviewed changes

[colors] "color_auto" is now managed by a specific ForegroundColorPro…

ea1a429

…perty descriptor

olivierphi commented Jul 6, 2022

View reviewed changes

willmcgugan closed this Aug 26, 2022

willmcgugan deleted the implement-color-auto branch February 6, 2023 09:44

		)


		def parse_color_expression(color_text: str) -> tuple[Color, bool]:

	"color", f"auto{f' {self.color.a * 100}%' if self.color.a < 1 else ''}"
	"color", f"auto {self.color.a * 100}%" if self.color.a < 1 else "auto"

[colors] Add a "auto" color #596

[colors] Add a "auto" color #596

Conversation

olivierphi commented Jun 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darrenburns left a comment

Choose a reason for hiding this comment

willmcgugan 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

olivierphi commented Jun 30, 2022

willmcgugan commented Jun 30, 2022

willmcgugan left a comment

Choose a reason for hiding this comment

willmcgugan Jun 30, 2022 • edited

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

olivierphi Jul 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivierphi commented Jul 1, 2022

willmcgugan 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

darrenburns commented Jul 1, 2022

willmcgugan commented Jul 1, 2022

olivierphi commented Jul 1, 2022

willmcgugan commented Jul 1, 2022

olivierphi commented Jul 1, 2022 • edited

List of the "universal" values we can apply to any CSS property in browsers' CSS:

willmcgugan commented Jul 1, 2022

olivierphi commented Jul 1, 2022

willmcgugan commented Jul 1, 2022

olivierphi commented Jul 1, 2022

willmcgugan commented Jul 1, 2022

olivierphi commented Jul 5, 2022

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

olivierphi commented Jun 29, 2022 •

edited

willmcgugan Jun 30, 2022 •

edited

olivierphi Jul 1, 2022 •

edited

olivierphi commented Jul 1, 2022 •

edited