Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix subheader fold propagation #440

Merged
merged 4 commits into from
Aug 29, 2020

Conversation

necto
Copy link
Contributor

@necto necto commented Aug 29, 2020

Addressing the bug found in #431:

If a user folds a header, all its subheaders should collapse as well, so that when the user reopens it, they stay closed.

The previous behavior is buggy in a way that it keeps the subheaders open as they were, restoring their openness when the header is unfolded.

The previous behavior is useful at least for two users of organice (@munen and myself), so this change set also introduces a user setting.

When closing a header with nested subheaders, it closes them too.
Let the user choose if they want to close the subheaders of a closed header the
way org-mode implements it or to preserve the openness of the subheaders,
restoring it on reopening the header.
@necto
Copy link
Contributor Author

necto commented Aug 29, 2020

Need help with this flaky test:
"Render all views › Org Functionality › Renders everything starting from an Org file › Agenda › Clicking the Timestamp in a TODO within the agenda toggles from the date to the time".
It has failed on my machine even on the current master. However when I started debugging it, in particular, when I inserted a bunch of prettyDOM logs and

console.log(queryByText(/12 months ago/));

right before the failing line, it succeeded and was succeeding ever since (on my machine).

Any idea what might be happening here?

@munen
Copy link
Collaborator

munen commented Aug 29, 2020

Need help with this flaky test:

Curious that you're saying it's flaky. That's good to know and I didn't so far. I'm not aware that this test ever failed on me or on CI.

But if it fails for you and then it works when you're debugging, that sounds like a timing issue to me. The test does open the agenda, which when run in the browser, does have an animation, at least. However, if that test fails for you sometimes, then other tests opening the agenda or similar modals (like search or the todo list) should also fail. Is that the case? Or is this the only test in the 'sometimes failing/flaky' category?

@munen
Copy link
Collaborator

munen commented Aug 29, 2020

Iirc this is the first time that this test fails on CI. But since it does, I'll rerun the build. Not to say that all is good if it doesn't fail, but to see if it's a 'sometimes' failing test for CI.

@munen
Copy link
Collaborator

munen commented Aug 29, 2020

NB: The test is green on my machine - on master and this feature branch.

@munen
Copy link
Collaborator

munen commented Aug 29, 2020

Ok, rebuild is green. So it seems to be flaky, indeed.

@munen
Copy link
Collaborator

munen commented Aug 29, 2020

Apart from the pre-existing flaky test, the new feature, implementation and tests look good to me!

I've added a bit of minor doc regarding what's the default in Org mode (503226f) and added your contribution to the changelog (dc557bc).

Since the test is green for me and on CI on a second try, and I've manually confirmed functionality of the agenda, I will merge this PR. I'll open an issue where we can discuss the options to improve the test reliability.

Great contribution as always, thank you for your continued effort, @necto 🙏 🙇

@munen munen merged commit ebca34b into 200ok-ch:master Aug 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants