zulip-terminal icon indicating copy to clipboard operation
zulip-terminal copied to clipboard

Changing the order of message links and topic links in popup(from #1238)

Open Kelp710 opened this issue 2 years ago • 6 comments

What does this PR do?
Changed the order of message links and topic links in popup. in order to achieve this, switched if statements in MsgInfoView class in views.py

Tested?

  • [x] Manually
  • [x] Existing tests (adapted, if necessary)
  • [ ] New tests added (for any new behavior)
  • [ ] Passed linting & tests (each commit)

Visual changes <image>

Kelp710 avatar Jul 01 '22 00:07 Kelp710

Hi! Im trying to do the testing part. However, I couldnt understand what "we could add a test for the existing order first, and then change the expected behaviour and update the test in the current commit you have now" mean. If you could, could you give me a little insight into it?

Kelp710 avatar Jul 03 '22 17:07 Kelp710

Hi! I'm trying to do the testing part. However, I couldn't understand what "we could add a test for the existing order first, and then change the expected behaviour and update the test in the current commit you have now" mean. If you could, could you give me a little insight into it?

I meant to add a separate first commit which would add a test for the object we're displaying, which would check the order of the sections matches what we expect. That new test should be expected to pass right now.

Then in the existing commit you have, you would update that new test you just added to ensure the new expected order instead.

If you need more detail or advice I'd suggest discussing in the stream topic on chat.zulip.org

neiljp avatar Jul 04 '22 00:07 neiljp

@Kelp710 Were you meaning to squash the last 2 "new" commits into the first test commit?

Rohitth007 avatar Jul 10 '22 03:07 Rohitth007

@Rohitth007 Thank you for mentioning the problem!

Kelp710 avatar Jul 10 '22 04:07 Kelp710

@Kelp710 Are you ready for further feedback on this?

neiljp avatar Jul 21 '22 02:07 neiljp

@neiljp @Rohitth007 Thank you for the message and feedback, I`ll try to fix it soon!

Kelp710 avatar Jul 23 '22 14:07 Kelp710

@Kelp710 Thanks for taking a look at this one previously :+1:

This was followed up in #1265, work I just split into the 2-commit style I requested here, ie. add a test for the current condition, then adjust the test and the code. I've adjusted both commits from there a little (particularly commit text), and primarily attributed the first commit to that author for following through with the PR and tidying the test, and left the second attributed to you.

This is now merged in the two commits ending 77218c1a646b431a463d3ab84ff61d92ceaf5c76 :tada: (check the parent commit to see the previous one, or update your main branch)

neiljp avatar Mar 08 '23 19:03 neiljp