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

Added global time element #1232

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

AshutoshKhadse23
Copy link

I had added the global time element

Related #1215

issue.1215.mp4

@AshutoshKhadse23
Copy link
Author

I added the global time element, please notify for any changes required.

@gnprice
Copy link
Member

gnprice commented Dec 30, 2024

The comment you got at #1231 (comment) on the previous version of this PR still applies:

Thanks for the PR! This needs to follow our Git style and exclude unrelated changes before we can review it.

When a PR needs changes, update the existing PR rather than close it and make a new one. That way discussion doesn't restart from scratch. For help, see the Zulip Git guide.

@AshutoshKhadse23
Copy link
Author

Hello @gnprice ,
Now I had added only necessary changes and removed the unnecessary changes.

@gnprice
Copy link
Member

gnprice commented Dec 31, 2024

Before asking for review, you need to take a look at your own changes and self-review.

This deletes a bunch of localizations, and it makes hundreds of lines of formatting changes. None of that is related to the feature this PR is described as being about. So the same feedback you were given before still applies.

@AshutoshKhadse23 AshutoshKhadse23 force-pushed the insert-time-element--global branch 3 times, most recently from fb4e848 to de2c0e1 Compare January 2, 2025 09:38
@AshutoshKhadse23 AshutoshKhadse23 force-pushed the insert-time-element--global branch from 2609713 to a75e921 Compare January 2, 2025 09:46
@AshutoshKhadse23 AshutoshKhadse23 force-pushed the insert-time-element--global branch from 90f437a to c28e1a4 Compare January 2, 2025 10:08
@AshutoshKhadse23
Copy link
Author

Hello @gnprice ,
I had made all the changes according to your comments ,could you review and tell for the changes.

@chrisbobbe
Copy link
Collaborator

This isn't a coherent sequence of well-explained changes:

image

For examples of mergeable commits, please browse the commit history: https://github.com/zulip/zulip-mobile/blob/main/docs/howto/git.md#2-the-secret-to-using-git-log--p

You're welcome to ask Git questions in #git help in the Zulip development community.

@AshutoshKhadse23
Copy link
Author

Hello @chrisbobbe ,
Extremely Sorry for that as it was my first contribution ,it took time me to be familiar with the environment .Should I create another pull request to properly represent the change?

@chrisbobbe
Copy link
Collaborator

No need to make a new PR; just force-push your new branch here.

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.

3 participants