Skip to content

#171 : Show edited/moved marker on messages #543

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

Closed
wants to merge 18 commits into from
Closed

Conversation

fizy069
Copy link

@fizy069 fizy069 commented Mar 1, 2024

Haven't added the zulip icons yet, since I wasn't able to run the script to auto generate the icons.dart file.

Fixes: #171

@fizy069
Copy link
Author

fizy069 commented Mar 1, 2024

CI failed probably because I did not add the generated files to the commit. Will do so soon.

@gnprice
Copy link
Member

gnprice commented Mar 1, 2024

Thanks for the contribution!

In addition to getting the existing tests passing, before we can review this PR in detail you'll need to write tests for it — see the note in the README file.

If you have questions as you try to do that, we're happy to discuss in #mobile-team on chat.zulip.org.

@gnprice
Copy link
Member

gnprice commented Mar 1, 2024

Please also post screenshots. That helps people review the UI changes.

@gnprice
Copy link
Member

gnprice commented Mar 4, 2024

I've edited the PR description to link it up with the issue, to make it easy to find when looking at the issue.

When the PR is ready for review, just post a comment here saying so and then I can take a look.

@fizy069
Copy link
Author

fizy069 commented Mar 8, 2024

#F171
Here is how it looks currently.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks for the revision!

Some quick comments below on the code so far.

One thing we'll need before we can fully review this PR is for the changes to be organized into clear and coherent commits. See the relevant README section:
https://github.com/zulip/zulip-flutter#submitting-a-pull-request

From the screenshots, it looks like this UI doesn't yet match the design in at least one important way: pay close attention to where the edited/moved markers are when at rest, before swiping to show the word "Edited" or "Moved".

Comment on lines +13 to +15
# Set fallback configurations for older versions of the flutter tool.
if (NOT DEFINED FLUTTER_TARGET_PLATFORM)
set(FLUTTER_TARGET_PLATFORM "windows-x64")
Copy link
Member

Choose a reason for hiding this comment

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

This change looks unrelated to the rest of the PR. As part of making clear and coherent commits:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html
it'll therefore need to be its own commit, with a commit message explaining why we want to make this change.

@fizy069 fizy069 marked this pull request as draft March 11, 2024 09:17
@fizy069 fizy069 closed this Apr 4, 2024
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.

Show edited/moved marker on messages
2 participants