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

Prettify number of issues #17760

Merged
merged 21 commits into from
Jun 12, 2022
Merged

Prettify number of issues #17760

merged 21 commits into from
Jun 12, 2022

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Nov 22, 2021

  • Use the PrettyNumber function to add commas in large amount of issues.

Before:
image

After:
image

@Gusted Gusted added the type/enhancement An improvement of existing functionality label Nov 22, 2021
@zeripath
Copy link
Contributor

Unfortunately you can't change the locale files other than English as they'll just get overridden by crowd in.

You'll need to back those out. Sorry.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 22, 2021
@zeripath
Copy link
Contributor

Does pretty number do the right thing for each locale?

To a lot of Europeans 1,502 means one point five oh two not one thousand five hundred and two.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 23, 2021

Many localization works should be done on browser side, use JS to format number/money/datetime.


Update: if we can have user's i18n preference, doing such format in backend is more easy.

@lunny
Copy link
Member

lunny commented Nov 23, 2021

Is this a habit for all countries or most countries?

@wxiaoguang
Copy link
Contributor

Is this a habit for all countries or most countries?

Many countries. eg: Indonesia also uses "1.000,5" for one thousand point five.

> console.log(new Intl.NumberFormat('id').format(1000.5));
1.000,5

@zeripath
Copy link
Contributor

zeripath commented Nov 23, 2021

India is particularly confusing because of the use of the lakh (100 000 to most of us) and crore (10 000 000 to most of us).

This means that 12 million 345 thousand 678 e.g. 12 345 678 (in most of the world) is actually considered to be 1 crore 23 lakh 45 thousand 678 eg.
1 23 45 678 and should be grouped as such.

Most Indians would use . as the decimal point and , as a units separator although space appears to be tolerated. (As it is in the rest of the world.)

@silverwind
Copy link
Member

silverwind commented Nov 23, 2021

The current approach has issues (it won't render as SI units and it does not follow html[lang] while it should), but I think I can offer a better method. @Gusted can you give me collaborator access to your gitea fork so I can push to your branches?

@Gusted
Copy link
Contributor Author

Gusted commented Nov 23, 2021

The current approach has issues (it won't render as SI units and it does not follow html[lang] while it should), but I think I can offer a better method. @Gusted can you give me collaborator access to your gitea fork so I can push to your branches?

Voila, apparently you need to become a collaborator on the repo itself - as individual branches don't seem to be a option.

@silverwind
Copy link
Member

Thanks, I have a few changes in mind, will follow up tomorrow.

@lafriks
Copy link
Member

lafriks commented Nov 24, 2021

Latvian language has 20 000,58 and 1000 without space

@Gusted
Copy link
Contributor Author

Gusted commented Nov 24, 2021

Latvian language has 20 000,58 and 1000 without space

new Intl.NumberFormat('lv').format(1000)
"1000"
new Intl.NumberFormat('lv').format(2000058)
"2 000 058"
new Intl.NumberFormat('lv').format("20000.58")
"20 000,58"

We won't be formatting any decimals(hopefully)

@silverwind
Copy link
Member

silverwind commented Nov 24, 2021

Should be ready from my side. The new version now formats on both server (english only) and client (based on current set language) and only updates the DOM when formats differ.

I also handled a few more cases like milestone counts and consolidated i18n entries to just one. Would like to rename it but I'm afraid that may not be possible without invalidating existing translations.

@silverwind
Copy link
Member

silverwind commented Nov 24, 2021

One issue that remains is that with JS formatting, there will be a brief flash of english data in other locales. It could be avoided using a render-blocking inline script on pages that have these tags, but I'm afraid our JS build tools can not easily do such a task. I guess I'll keep it in mind for a future improvment.

modules/base/tool.go Outdated Show resolved Hide resolved
@zeripath
Copy link
Contributor

As an aside are we rendering dates and times correctly as per locale?

(I don't think we render dates in MM/DD/YYYY format because that would definitely have annoyed me enough to make me fix things.)

@silverwind
Copy link
Member

As an aside are we rendering dates and times correctly as per locale?

Don't think we do. Last I checked date rendering is horrendous, both that it does not format according to locale and also hardcodes to the server's time zone. It's certainly something that needs to be looked at. The issue is mostly obscured by the fact that most of the UI renders relative dates, absolute ones only appear in tooltips.

@silverwind
Copy link
Member

Should probably make a sub-template for these number <span>s. The same method could be used for dates and SI-formatted numbers later too.

@silverwind
Copy link
Member

Few more tweaks done:

  • Introduce template helper for these number
  • Fixed various octicons on these open/close buttons

Screen Shot 2021-11-27 at 00 07 45

Ready from my side.

Co-authored-by: silverwind <me@silverwind.io>
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

Probably good enough to land this now and iterate upon it later.

@lunny
Copy link
Member

lunny commented May 16, 2022

Please resolve the conflicts.

@Ryuno-Ki
Copy link
Contributor

It looks like some merge conflicts found their way back :-(

@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@edf1420). Click here to learn what that means.
The diff coverage is 70.00%.

@@           Coverage Diff           @@
##             main   #17760   +/-   ##
=======================================
  Coverage        ?   47.28%           
=======================================
  Files           ?      966           
  Lines           ?   134135           
  Branches        ?        0           
=======================================
  Hits            ?    63425           
  Misses          ?    62989           
  Partials        ?     7721           
Impacted Files Coverage Δ
modules/util/util.go 76.25% <57.14%> (ø)
modules/base/tool.go 92.95% <100.00%> (ø)
modules/templates/helper.go 44.33% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edf1420...0d18f79. Read the comment docs.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 12, 2022
@lunny lunny modified the milestones: 1.18.0, 1.17.0 Jun 12, 2022
@lunny
Copy link
Member

lunny commented Jun 12, 2022

make L-G-T-M work

@lunny lunny merged commit 796c4ec into go-gitea:main Jun 12, 2022
@Gusted Gusted deleted the pretty-number branch June 12, 2022 12:08
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 12, 2022
* giteaoffical/main:
  Fix signal loop in graceful manager (go-gitea#19943)
  Prettify number of issues (go-gitea#17760)
  Improve file header on mobile (go-gitea#19945)
  Unify repo settings & show better error (go-gitea#19828)
  [skip ci] Updated translations via Crowdin
  fixed comment typo (go-gitea#19944)
  Auto merge pull requests when all checks succeeded via WebUI (go-gitea#19648)
  Fix some mirror bugs (go-gitea#18649)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
* Prettify number of issues

- Use the PrettyNumber function to add commas in large amount of issues.

* Use client-side formatting

* prettify on both server and client

* remove unused i18n entries

* handle more cases, support other int types in PrettyNumber

* specify locale to avoid issues with node default locale

* remove superfluos argument

* introduce template helper, octicon tweaks, js refactor

* Update modules/templates/helper.go

* Apply some suggestions.

* Add comment

* Update templates/user/dashboard/issues.tmpl

Co-authored-by: silverwind <me@silverwind.io>

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
lunny pushed a commit that referenced this pull request Feb 17, 2023
…mplates (#22861)

This PR follows:
* #21986
* #22831

This PR also introduce customized HTML elements, which would also help
problems like:
* #17760
* #21429
* #21440

With customized HTML elements, there won't be any load-search-replace
operations, and it can avoid page flicking (which @silverwind cares a
lot).

Browser support:
https://developer.mozilla.org/en-US/docs/Web/API/Window/customElements

# FAQ

## Why the component has the prefix?

As usual, I would strongly suggest to add prefixes for our own/private
names. The dedicated prefix will avoid conflicts in the future, and it
makes it easier to introduce various 3rd components, like GitHub's
`relative-time` component. If there is no prefix, it's impossible to
introduce another public component with the same name in the future.

## Why the `custcomp.js` is loaded before HTML body? The `index.js` is
after HTML body.

Customized components must be registered before the content loading.
Otherwise there would be still some flicking.

`custcomp.js` should have its own dependencies and should be very light,
so it won't affect the page loading time too much.

## Why use `data-url` attribute but not use the `textContent`?

According to the standard, the `connectedCallback` occurs on the
tag-opening moment. The element's children are not ready yet.

## Why not use `{{.GuessCurrentOrigin $.ctx ...}}` to let backend decide
the absolute URL?

It's difficult for backend to guess the correct protocol(scheme)
correctly with zero configuration. Generating the absolute URL from
frontend can guarantee that the URL is 100% correct -- since the user is
visiting it.

# Screenshot

<details>

![image](https://user-images.githubusercontent.com/2114189/218256757-a267c8ba-3108-4755-9ae5-329f1b08f615.png)

</details>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.