feat: make stepper bubbles collapse and refine css working #702

EdoStorm96 · 2024-09-19T14:00:58Z

So, I was just puzzling a bit with the stepper, and to my surprise, came up with a solution for making the bubbles collapse ...

This is still a bit rough, but I thought I'd share it with you before the (my) weekend. Will add more notes on monday!

miggol

Although I'm impressed with how you've got this feature working with so few lines, I'm not happy with the implementation. There are two main reasons for this.

First, I believe the representation of the stepper produced by build_stepper() should be "correct" and complete. Right now, we are collapsing stepper items by completely ommitting children that do exist in stepper.items. This means that there are items with parents that don't have children. Also, the resulting stepper's shape is very different depending on which item is current. This makes it unnecessarily hard to reason and potentially debug the output of build_stepper

Of course, the css classes and such are also different depending on which item is current, but I consider that to be part of the presentation rather than the internal representation. Collapsing of stepper items should in my view also be part of the presentation, not the internal representation.

Second, but somewhat related, this implementation is not very flexible. This implementation only works for top-level items: collapsing children of a child would get messy very quickly. We might also want to move to JS-expandable stepper items in the future, but ommitting children entirely does not allow for that.

My recommendation would be to keep the collapsing part of the stepper to the presentation side of things, in this case I would suggest moving it to the template:

{% if not item.is_collapsed %}
    {% if item.children %}
        ... Render children ...
    {% endif %}
{% endif %}

The is_collapsed flag can easily be repurposed into a CSS class if we move to JS collapsing. Also, we can inspect the complete structure of the stepper from within a breakpoint without needing to worry about children being ommitted.

What I do think we should keep is the one-time search for a current item, that's a great optimization. I don't think it belongs in build_stepper() which I would rather keep clean if possible. I would recommend a separate loop, or at least a separate method that checks for currentness.

miggol · 2024-09-23T10:03:44Z

proposals/utils/stepper.py

        for item in self.items:
+            #Only check item.is_current until there is a current item found
+            while not self.current_item:


I love that this makes sense in English but programmatically this while loop makes no sense. Anything below it will only ever run once so it should be an if statement.

miggol · 2024-09-23T10:06:24Z

proposals/utils/stepper_helpers.py

-            )
-        return " ".join(classes)
+
+    def get_ancestor(self):


miggol · 2024-09-23T10:22:42Z

proposals/utils/stepper_helpers.py

            return True
        return False

    def is_disabled(self):
        if not self.get_url():
            return True
        return False
+
+    def get_css_classes(self):


When writing a method that might be overwritten, it should have the same signature (i.e. return value types) as whatever might overwrite it.

A method that just passes returns None, and that is not the same as a string. So you might get an error if you do super().get_css_classes() + my_css_class.

It should in this case return an empty string. Or an empty list which you append to, and then finally you turn the list into a space-separated string somewhere else.

miggol · 2024-09-23T11:12:49Z

proposals/utils/stepper.py

+                    self.current_item_ancestor = item.get_ancestor()
+                else:
+                    break
+            item.get_css_classes()


What is the reason that we need to get the CSS classes at this stage? Can't we just call item.get_css_classes() from the template?

miggol · 2024-09-23T11:15:16Z

proposals/utils/stepper_helpers.py

        child_errors = []
        for child in self.children:
            child_errors += child.get_errors()
        if child_errors != []:
-            css_classes += "incomplete"
+            self.css_classes += "incomplete"


There isn't enough consistency in where the space goes. I think we might better avoid the issue entirely and provide a list of css classes, and only turn them into a string when needed for HTML.

EdoStorm96 · 2024-09-24T13:19:47Z

So here is the new version of this PR, some notes on what has been changed:

StepperItem's now have an is_available attribute. This is false by default. This relates to whether its children are displayed. A more correct name would be is_not_collapsed` ...
The new way of searching for the current item is retained, but this now happens in a separate method (item_is_current_check()) of the stepper, until a current item is found.
When a current item is found, its get_ancestors() gets called, which returns a list of the item + all of its ancestors (so parent's, parent.parent's etc) These items' is_available are then set to True
The get_errors function and get_css_classed have had a bit of an overhaul. get_errors, can now get an include_children boolean argument, so that the errors of an item's descendants are also taken into account.
The current design is, when an item is collapsed, its completeness gets evaluated with include_children, when an item is_availabe just the model_form errors and the optional get_checker_errors get evaluated.

This feels right to me :)

miggol

So here is the new version of this PR, some notes on what has been changed:

* StepperItem's now have an `is_available attribute. This is false by default. This relates to whether its children are displayed. A more correct name would be `is_not_collapsed` ...

How about is_expanded? No biggie either way

* The new way of searching for the current item is retained, but this now happens in a separate method (`item_is_current_check()`) of the stepper, until a current item is found.

* When a current item is found, its `get_ancestors()` gets called, which returns a list of the item + all of its ancestors (so parent's, parent.parent's etc) These items' `is_available` are then set to True

See my code comment, I think it would be better if the current check or build_css_classes would check if it's the current item and set the css class there, rather than in is_current().

Otherwise this is great.

* The get_errors function and get_css_classed have had a bit of an overhaul. get_errors, can now get an `include_children` boolean argument, so that the errors of an item's descendants are also taken into account.

* The current design is, when an item is collapsed, its completeness gets evaluated with `include_children`, when an item `is_availabe` just the model_form errors and the optional `get_checker_errors` get evaluated.

This feels right to me :)

Perfect!

miggol · 2024-09-30T13:09:23Z

proposals/utils/stepper_helpers.py

        """
        if request.path == self.get_url():
+            self.css_classes.add("active")


This method was previously stateless, being more of a "check" and not an action with consequences. If we add the CSS class here, we have to remember to do that for every time we write a custom is_current(), like in the Session item (which currently doesn't do this correctly).

I see how this makes sense, but I would prefer if the css class gets added in is_current_check() so we don't have so many different places to think about CSS classes.

miggol · 2024-09-30T13:12:29Z

proposals/utils/stepper.py

@@ -36,6 +36,8 @@ def __init__(
        # which item is current
        self.request = request
        self.items = []
+        self.current_item_ancestors = None


To keep consistent with types, this should be an empty list.

miggol · 2024-09-30T13:20:00Z

main/templates/base/stepper.html

                        <span class="stepper-bubble {{ bubble_size.0 }}">{% counter counter value %}</span>
                        <span>{{ item.title }}</span>
                    </a>
                {% endif %}
-                {% if item.children %}
+                {% if item.children and item.is_available %}


I like this collapsing implementation a lot more!

If in testing we decide, for example, that we want to leave the Trajectories items expanded by default, we can easily implement that. Very flexible.

EdoStorm96 · 2024-10-01T09:39:22Z

So, I've adressed your comments. Good ideas!

I also noticed that FundingForm did not really include validation, so I just added some cuz it was bugging me ...

miggol

Very nice! I have some small suggestions but functionally this works very well. Approving.

miggol · 2024-10-01T14:15:19Z

proposals/utils/stepper_helpers.py

+            while parent.parent:
+                parent = parent.parent


This is my favourite part of this PR

miggol · 2024-10-01T14:20:13Z

proposals/utils/stepper_helpers.py

+    def get_errors(self, include_children=False):
        """
-        By default, this returns the errors of the instantiated form, which get
-        evaluated as a boolean. By passing a custom get_checker_errors function
-        from the checker, custom errors can be specified also.
+        By default, this returns the errors of the instantiated form and,
+        optionally, its children. This then gets evaluated as a boolean.
+        By passing a custom get_checker_errors function
+        from the checker, custom errors also be added.
        """
-        if hasattr(self, "get_checker_errors"):
-            return self.get_checker_errors() or self.model_form.errors
-        return self.model_form.errors
+        errors = []
+        errors += self.model_form.errors

-    def css_classes(self):
-        css_classes = super().css_classes()
+        if hasattr(self, "get_checker_errors"):
+            errors += self.get_checker_errors()
+        if include_children and self.children:
+            for child in self.children:
+                errors += child.get_errors(include_children=True)
+        return errors


I feel like there's some unnecessary repetition here. Can't we just start with super().get_errors(include_children=include_children) and append the checker errors to that?

No, I don't think so because the super() just goes to StepperItem's get_errors which just returns an empty list. This is where the behaviour of this function gets specified. Up the chain, include children becomes basically meaningless.

miggol · 2024-10-01T14:21:57Z

proposals/utils/stepper_helpers.py

+        if not self.is_expanded and self.children:
+            if self.get_errors(include_children=True):
+                self.css_classes.add("incomplete")
+            else:
+                self.css_classes.add("complete")
+        elif self.get_errors():
+            self.css_classes.add("incomplete")
        else:
-            css_classes += " complete"
-        return css_classes
+            self.css_classes.add("complete")


I also feel like this could be simplified now that we have the is_expanded variable.

Wouldn't something like if self.get_errors(include_children=self.is_expanded) remove the need for the double if statements?

I might be missing something though, just a quick suggestion.

miggol · 2024-10-01T14:26:13Z

proposals/utils/stepper_helpers.py

+        is currently on and adds the active class to the item's
+        self.css_classes set. This gets called when building the stepper,


This comment is no longer accurate

feat: make stepper bubbles collapse and refine css working

9f63896

EdoStorm96 requested a review from miggol September 19, 2024 14:01

miggol requested changes Sep 23, 2024

View reviewed changes

EdoStorm96 added 3 commits September 23, 2024 15:22

Merge branch 'major/4' into feature/stepper_collapse

fcee331

fix: improve collapsible logic and stepperitem css classes

42dd918

style: style

e993fe2

EdoStorm96 marked this pull request as ready for review September 24, 2024 13:09

EdoStorm96 requested a review from miggol September 24, 2024 13:09

EdoStorm96 changed the base branch from feature/stepper_refinement to major/4 September 30, 2024 07:49

Merge branch 'major/4' into feature/stepper_collapse

383be30

miggol requested changes Sep 30, 2024

View reviewed changes

EdoStorm96 added 5 commits October 1, 2024 11:17

fix: rename is_available to is_expanded

35c7e74

fix: self.current_item_ancestors init's as empty list

dc1dec1

feat: add active css class in Stepper.item_is_current_check()

8ebd455

feat: add validation to FundingForm

0b715db

style: style

8d955e6

EdoStorm96 requested a review from miggol October 1, 2024 09:39

miggol approved these changes Oct 1, 2024

View reviewed changes

EdoStorm96 added 2 commits October 2, 2024 10:39

fix: update docstring

a39e015

fix: simplyfy ModelFormItem.build_css_classes()

8a61a61

EdoStorm96 merged commit f27ba9a into major/4 Oct 7, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make stepper bubbles collapse and refine css working #702

feat: make stepper bubbles collapse and refine css working #702

EdoStorm96 commented Sep 19, 2024

miggol left a comment

miggol Sep 23, 2024

miggol Sep 23, 2024

miggol Sep 23, 2024

miggol Sep 23, 2024

miggol Sep 23, 2024

EdoStorm96 commented Sep 24, 2024

miggol left a comment

miggol Sep 30, 2024

miggol Sep 30, 2024

miggol Sep 30, 2024

EdoStorm96 commented Oct 1, 2024

miggol left a comment

miggol Oct 1, 2024

miggol Oct 1, 2024

EdoStorm96 Oct 2, 2024

miggol Oct 1, 2024

miggol Oct 1, 2024

		is currently on and adds the active class to the item's
		self.css_classes set. This gets called when building the stepper,

feat: make stepper bubbles collapse and refine css working #702

feat: make stepper bubbles collapse and refine css working #702

Conversation

EdoStorm96 commented Sep 19, 2024

miggol 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

EdoStorm96 commented Sep 24, 2024

miggol 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

EdoStorm96 commented Oct 1, 2024

miggol 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