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

bugfix: ui: Focus on content_area when draft is opened from side-panels. #1044

Merged
merged 2 commits into from
Jun 3, 2021

Conversation

zee-bit
Copy link
Member

@zee-bit zee-bit commented Jun 1, 2021

This commit fixes a bug that caused the focus to not move to the content
area if the draft was opened from the side-panels. This is minor bugfix
that should set the focus to the message_column before setting it to
the footer(i.e. content_area).

@zulipbot zulipbot added the size: XS [Automatic label added by zulipbot] label Jun 1, 2021
@neiljp
Copy link
Collaborator

neiljp commented Jun 1, 2021

@zee-bit This is a nice simple fix 👍 Are there tests for this that can be amended to ensure this doesn't break?

I'm happy to merge after we get clarification on a test. However, a good follow-up refactor would be to find some of the similar code associated with opening the compose box and ensure we do the same actions each time by wrapping it in new methods. That may also be useful for resolving things like #1042, though we'd need to check if that's quite the right solution.

@neiljp neiljp added the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Jun 1, 2021
@zee-bit
Copy link
Member Author

zee-bit commented Jun 2, 2021

Thanks for looking at my PR, Neil.

I grep'ed our codebase for drafts, and couldn't find any tests for opening draft compose, so there was nothing to amend. I'll add the tests for this in another commit in this PR(just to keep the bugfix isolated).
Since this topic is brought up, I'd also like to remind that there's an open PR that adds tests for SAVE_AS_DRAFT in #950. Although that is not related to this PR in general we might want to address that PR too?

@zee-bit zee-bit force-pushed the bugfix-draft-focus branch 2 times, most recently from ab5a0b0 to c4d3693 Compare June 2, 2021 15:04
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: XS [Automatic label added by zulipbot] labels Jun 2, 2021
@zee-bit zee-bit added PR needs review PR requires feedback to proceed and removed PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Jun 2, 2021
@zee-bit
Copy link
Member Author

zee-bit commented Jun 2, 2021

@neiljp I had already updated this PR, if you wanted to re-review.

I've added the tests for OPEN_DRAFT keypress as the first commit such that it will fail, and the second commit with the one-liner bugfix will fix the test eventually. Let me know if there's anything else that needs to be added/updated. :)

Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zee-bit Thanks for adding the test! 👍

Rather than having the first commit break, I'd suggest adding the bulk of
the test for the current behavior in the first commit - which should pass -
and then amending that test and adding the new behavior in the second
commit.

The test logic is rather complex with the mocks, but if we can refactor the
enter-compose functionality this should become much simpler, I expect.

@zee-bit
Copy link
Member Author

zee-bit commented Jun 3, 2021

The tests are a bit complex currently, I admit. But I am not sure if we should do the refactoring in this PR too. That would also require changing it for all enter-compose functionality, not just draft.

The other way, we could get rid of those mocks or at least reduce the apparent redundancy(since other tests define similar mocks too) is to define them all once in the init function?

btw, updated the PR according to your first point above.

This commit fixes a bug that caused the focus to not move to the central
area if the draft was opened from the side-panels. This is a minor bugfix
that helps redirect the focus to the compose area in such cases.

The recently added tests for OPEN_DRAFT are amended in this commit to
match the added bugfix.
@neiljp neiljp force-pushed the bugfix-draft-focus branch 2 times, most recently from 39f152c to 472b623 Compare June 3, 2021 15:41
@neiljp
Copy link
Collaborator

neiljp commented Jun 3, 2021

@zee-bit Thanks for rearranging the test elements - that was my main interest for getting this merged initially, which I'm just about to do after some trimming to your commit text (each commit is independent so doesn't need to refer to the other) 🎉

If you want to take up the refactor please could you note that or open an issue?

@neiljp neiljp merged commit 8f3b193 into zulip:main Jun 3, 2021
@neiljp neiljp added this to the Next Release milestone Jun 3, 2021
@zee-bit
Copy link
Member Author

zee-bit commented Jun 3, 2021

@neiljp Thanks for merging.
Noted. I'll create a PR for the refactor soon...

@neiljp neiljp added the bug Something isn't working label Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working PR needs review PR requires feedback to proceed size: L [Automatic label added by zulipbot]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants