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

Use locale for by the numbers animation #606

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

omsuneri
Copy link
Contributor

updating the counter animation which now displays the numbers with commas previously it is just displaying the numbers without commas @quozl @pikurasa review it
Screenshot 2024-12-21 at 11 19 19 AM

…mmas previously it is just displaying the numbers without commas
@omsuneri
Copy link
Contributor Author

@pikurasa @quozl i had created a new PR with the new commit message review it please

@quozl
Copy link
Contributor

quozl commented Dec 23, 2024

I had limited time and stopped after reading the commit message. Please follow our guidance, and the git convention of restricting the length of the first line. GitHub correctly splits the commit message midword to indicate the betrayal of convention. See 67069b5 rendered by GitHub.

@quozl
Copy link
Contributor

quozl commented Dec 24, 2024

Add backlink to prior discussion; #600 outstanding comments were;

  • rewrite commit messages according to project guidance,
  • respond if possible to our concern over commas not used in some cultures; (this is a browser side feature, and toLocalString probably handles this, so chances are you just have to stop mentioning commas in the commit message and don't explain your change with screenshots),

In future you can and should update the existing pull request rather than create a new one. Just change your branch HEAD and force push. That will keep the discussion in one place, and minimise notifications to other developers. 😁

@omsuneri
Copy link
Contributor Author

@quozl I got it now I ll be updating the commit message again without making new pr thanks 😊

@quozl
Copy link
Contributor

quozl commented Dec 24, 2024

Please resolve conflicts (i.e. once I saw there was conflict, there was no need for further review at this time).

@quozl quozl changed the title Updating the number counter animation Use locale for by the numbers animation Dec 25, 2024
@omsuneri
Copy link
Contributor Author

@pikurasa i m not getting why there are so many conflicts happening with this part of code now the animation is not working in my device after the latest pull from master

@quozl
Copy link
Contributor

quozl commented Dec 28, 2024

Thanks. That's interesting. Perhaps you pulled from master causing a merge but without doing a rebase.

GitHub does not express divergence and merges well, which is why we recommend rebasing, squashing, and generally avoiding merge commits.

Here's what I did to diagnose your situation, using my own repository clone;

  • fetch the commits from this pull request, git fetch origin pull/606/head
  • switch to the head of your pull request branch, git checkout e264e2e, (later on I'll recover from that by switching back to the master branch, git checkout master),
  • use Git visualisation tools such as git log an gitk to examine the history at that point.

Looking at your commit e264e2e it is a merge of your previous commit 67069b5 with three other commits that were on master branch; f80021f, 2e4aa43, and b5ee41f.

Your change is being made to js/custom.js

Of these commits, only 2e4aa43 changes this file, so this commit is where a merge must occur.

Unfortunately, we let through from @saumyashahi some unnecessary whitespace changes. Your change also has unncessary whitespace change. We should all be more careful in future.

Dropping your merge commit (git reset --hard 67069b5) and then retrying a rebase (git pull --rebase origin master) does enter a merge conflict, which is a helpful state in which Git guides you on what to fix.

It was easy for me to edit the file, choose which of the conflicted sections to keep, and then git add js/custom.js and git rebase --continue . I suggest you do the same, perhaps in another branch to practice. Once you are happy with the changes you have made, use git push --force to update the pull request.

Now a lot of people bail out at this point and start a new pull request, but that's the wrong way to solve the problem, and is impolite as it causes notifications and splits the discussion. So please keep that out of your mind as an option.

That said, while doing the merge by hand I noticed again your commit message was not correctly written. Please do fix it before we merge.

written by a human

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