-
Notifications
You must be signed in to change notification settings - Fork 19
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
issue 826: show per-source ballot counts #835
Conversation
b56b94e
to
35b1fe2
Compare
63785a0
to
5341b7e
Compare
35b1fe2
to
00a2c34
Compare
5341b7e
to
e87e726
Compare
0d2fbc0
to
a4f788e
Compare
perSourceCvrCountTable.getItems().add(new Pair<>(fileString, countString)); | ||
maxFilenameLength = Math.max(maxFilenameLength, fileString.length()); | ||
} | ||
perSourceCvrColumnFilepath.setPrefWidth(getEstWidthForStringInPixels(maxFilenameLength)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any risk of the same file name in different folders? Should we show the full path? If we do, it looks even harder to read.
One way you could handle this is by making the columns resizable by the user, and having this column justify right and set to a fixed width (so it first shows them the file name, and if they expand it, it can show the path). Alternatively, you could split the filename into one column, and the path into a separate one (maybe to the right of it).
Defer to @yezr on whether or not this is worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is currently resizeable by the user, and I tried that at first but I found that manually expanding a cell in a horizontally-scrolling window is actually kind of hard: you're expanding, but so is the container, so it scrolls -- so you're resizing controls three things: the cell size, the container size, and the scroll position. This felt clunkier than the current solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sounds good. 👍
data.discard(); | ||
lastLoadedCvrData = data; | ||
}; | ||
|
||
watchGenericService(service, onSucceededEvent); | ||
} | ||
|
||
private float getEstWidthForStringInPixels(int stringLength) { | ||
return stringLength * 7.5f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this varies given OS, monitor specs, magnification accessibility settings, etc.? Is there no autosize option we could use for a column instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a protected method that resizes, but doing that feels so much clunkier than this estimate: https://stackoverflow.com/a/38686402
This estimate is likely to make a cell that's too wide, but that's not really an issue. It should never make a cell that's too small. I think it's dependent on screen density but not any other factors, though I haven't tested on accessibility string scaling (and wonder if our UI works at all there?)
Note that the column is resizable so the estimate doesn't need to be perfect. LMK if you prefer the method in that stackoverflow link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine as-is, thanks!
data.discard(); | ||
lastLoadedCvrData = data; | ||
}; | ||
|
||
watchGenericService(service, onSucceededEvent); | ||
} | ||
|
||
private float getEstWidthForStringInPixels(int stringLength) { | ||
return stringLength * 7.5f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine as-is, thanks!
perSourceCvrCountTable.getItems().add(new Pair<>(fileString, countString)); | ||
maxFilenameLength = Math.max(maxFilenameLength, fileString.length()); | ||
} | ||
perSourceCvrColumnFilepath.setPrefWidth(getEstWidthForStringInPixels(maxFilenameLength)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sounds good. 👍
closes #826
This UI feels quite cluttered now. The table only appears after the first button is hit, so on pop-up it's cleaner.
This auto-sizes the filename column to try to make sure the full string is visible