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

Prettier poll result #245

Merged
merged 16 commits into from
Apr 17, 2024

Conversation

hiroshisuga
Copy link
Contributor

@hiroshisuga hiroshisuga commented Jun 27, 2023

Before:
Image-0

After:
Image2-1

, expecting having more than 100 votes in an option is rare, and a question having more than 10 options would not happen.

Update -> use more general algorithm, so that more than 10 question options and more than 100 votes are allowed, even though it would not happen for now (it may happen in the future, who knows..)
スクリーンショット 2023-06-27 21 25 47

@ffdixon
Copy link
Member

ffdixon commented Jun 27, 2023

This does look much nicer!

@hiroshisuga hiroshisuga marked this pull request as draft June 27, 2023 11:46
@hiroshisuga hiroshisuga marked this pull request as ready for review June 27, 2023 12:36
@hiroshisuga
Copy link
Contributor Author

Please try with other locales. I tested it only with Japanese locale.

@hiroshisuga hiroshisuga marked this pull request as draft June 27, 2023 13:21
@hiroshisuga hiroshisuga marked this pull request as ready for review June 27, 2023 13:35
@hiroshisuga hiroshisuga marked this pull request as draft June 27, 2023 13:56
@hiroshisuga
Copy link
Contributor Author

hiroshisuga commented Jun 27, 2023

I realised that on Webkit browsers including Firefox it did not work perfectly even though quite improved... needs more investigations...
On mobile Chrome it works.

@hiroshisuga hiroshisuga marked this pull request as ready for review June 28, 2023 01:37
@hiroshisuga
Copy link
Contributor Author

I think the problems are all fixed. Safari and Firefox show the poll correctly.

@hiroshisuga
Copy link
Contributor Author

Well, I've found the on Ubuntu the full block character is not really the same size with digit characters. Needs a bit more tweaking..

@hiroshisuga
Copy link
Contributor Author

Now Chrome on Linux works perfectly. Firefox on Linux is much better, but not perfect. This issue seems more complicated than I thought...

Copy link
Contributor

@GuiLeme GuiLeme left a comment

Choose a reason for hiding this comment

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

Great PR, I just have a minor thing to point out:
image
In this test I did, the beginning of the bar is slightly off, maybe it has something to do with the different spaces, I don't know. If there is no way to make it aligned, I'd say no problem.

Also, there is the other comments I left out

src/utils/data/index.js Outdated Show resolved Hide resolved
@@ -40,6 +42,8 @@ const FULL_BLOCK = '█';
const LEFT_HALF_BLOCK = '▌';
const RIGHT_HALF_BLOCK = '▐';
const EMPTY_BLOCK = '-';
const FIGURE_SPACE = ' ';
const EN_SPACE = ' '
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity, what is the difference between the EN and the Figure spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some experiment, I've found FIGURE_SPACE does not work for Firefox and EN_SPACE is better. I don't know the difference in detail, but by definition EN_SPACE is 1/2 em. And FIGURE_SPACE should have the same size of digit zero of a fixed font. However, there are somebody saying that it's not guaranteed....

@hiroshisuga
Copy link
Contributor Author

@GuiLeme , what is the OS and browser that you found the unaligned poll bar?

Co-authored-by: Guilherme Pereira Leme <[email protected]>
@GuiLeme
Copy link
Contributor

GuiLeme commented Jul 20, 2023

@hiroshisuga, It was a chrome (chrome version 114.0.5735.133, I don't know if this is an important information) on ubuntu 22.04

@hiroshisuga
Copy link
Contributor Author

Hi @GuiLeme , unfortunately, I cannot reproduce the phenomenon on my Ubuntu 22.04 with Chrome and Brave. It could be a locale problem...

const getPads = (n) => {
if (deviceInfo.osName === "Linux") {
if (browserInfo.isChrome) {
return FIGURE_SPACE.repeat(n);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GuiLeme , can you try change it either to
return FULL_BLOCK.repeat(n);
or
return EN_SPACE.repeat(n);
and check if your problem persists?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, @hiroshisuga, I tested with both as requested and the EN_SPACE worked pretty well!! here is the result:
image

The other one, FULL_BLOCK, didn't work, it broke the alignment even more.

Could you test if the EN_SPACE works for you as well? And then, perhaps remove the browser verification on line 73, because the same EN_SPACE goes for Firefox.

Copy link
Contributor Author

@hiroshisuga hiroshisuga Jul 24, 2023

Choose a reason for hiding this comment

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

Hi @GuiLeme , in my Ubuntu box (22.04) it (EN_SPACE) did not work. With Brave browser it look like:
image

It could be a locale problem (though I am using English).... let me leave it here and wait for bug reports.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hiroshisuga, Hum, yeah, there must be a problem with locale, I don't think mine is set to English. But as I said, I guess it is just fine the way it is. I don't think this should be a blocking problem.

Let's see what @antobinary thinks about this. I guess everything is fine! If everyone agrees, I can approve the PR!

Copy link
Contributor

@GuiLeme GuiLeme left a comment

Choose a reason for hiding this comment

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

Just that change and we are good to go!

src/utils/data/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@GuiLeme GuiLeme left a comment

Choose a reason for hiding this comment

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

LGTM!

@antobinary antobinary merged commit a51c962 into bigbluebutton:develop Apr 17, 2024
1 check passed
@antobinary
Copy link
Member

Thank you both!

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.

4 participants