Feature/na 356 Screenshot dialog UI #40
Conversation
vidhya-metacell
commented
Oct 15, 2024
vidhya-metacell
commented
|
@vidhya-metacell do you think we could make the warning text a little smaller?
Screencast.from.17-10-24.17.17.19.webm |
src/ui/screenshot_menu.css
Outdated
| @@ -14,25 +14,189 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| * { | |||
There was a problem hiding this comment.
The wildcard selector and .overlay change have adverse affects on the rest of the UI, we will need to move them into the specific sections of this .css file that they are needed, and remove the global wildcard selector and overlay changes
src/ui/screenshot_menu.ts
Outdated
| @@ -326,16 +401,28 @@ export class ScreenshotDialog extends Overlay { | |||
| this.statisticsTable.classList.add( | |||
| "neuroglancer-screenshot-statistics-table", | |||
| ); | |||
| this.statisticsTable.title = "Screenshot statistics"; | |||
| this.statisticsTable.title = "Screenshot progress"; | |||
There was a problem hiding this comment.
I think we can remove the occurences of setting .title in this file. Now that the UI is better designed, this info on hover is not really needed
src/ui/screenshot_menu.ts
Outdated
| const descriptionLearnMoreLink = document.createElement("a"); | ||
| descriptionLearnMoreLink.text = "Learn more"; | ||
|
|
||
| descriptionkeyHeader.appendChild(descriptionLearnMoreLink); |
There was a problem hiding this comment.
This docs page is not ready yet, so let's comment out creating the learn more link, and put a comment above it that this can be used to point to a docs page when complete
| cursor: pointer; | ||
| } | ||
|
|
||
| .neuroglancer-screenshot-filename-and-buttons { | ||
| margin-bottom: 5px; | ||
| padding: 1.5rem 1rem 1.25rem; |
There was a problem hiding this comment.
The screenshot menu design probably has a little too much vertical padding in hindsight, causing cases where you need to scroll to get to the buttons
Let's reduce some vertical padding to make it a bit tighter and reduce the likelihood the user needs to scroll to get to the screenshot buttons
src/ui/screenshot_menu.css
Outdated
| color: var(--gray200); | ||
| font-weight: 590; | ||
| } | ||
| .screenshot-scale-factor { |
There was a problem hiding this comment.
sorry this is my fault for not being clearer, apologies! We should change .screenshot-X to be .neuroglancer-screenshot-X to be consistent in naming
src/ui/screenshot_menu.css
Outdated
| align-items: center; | ||
| gap: 10px; | ||
| } | ||
| .neuroglancer-screenshot-tooltip { |
There was a problem hiding this comment.
we have some multiple definitions of the tooltip declaration. Let's combine these into a single declaration and do the same for the other classes
|
|
||
| .neuroglancer-screenshot-dialog .neuroglancer-screenshot-close-button { | ||
| background: url('../../src/ui/images/close.svg') no-repeat; | ||
| text-indent: -9999px; |
There was a problem hiding this comment.
We can remove the text content in the typescript code instead and remove this css spec
src/ui/screenshot_menu.css
Outdated
| align-items: center; | ||
| gap: 0.15rem; | ||
| } | ||
| .neuroglancer-screenshot-tooltip.screenshot-tooltip { |
There was a problem hiding this comment.
The name of this one is a bit confusing, is it getting used?
src/ui/screenshot_menu.css
Outdated
| margin-right: 2.125rem; | ||
| } | ||
| .neuroglancer-screenshot-scale-menu .neuroglancer-screenshot-scale-radio { | ||
| -webkit-appearance: none; |
There was a problem hiding this comment.
why do we need the -webkit-appearance change? Is there something that happens on chrome/safari that doesn't happen on firefox that we are accounting for here?
src/ui/screenshot_menu.css
Outdated
| } | ||
|
|
||
| .neuroglancer-screenshot-tooltip { | ||
| user-select: none; | ||
| background: url('../../src/ui/images/help_outline.svg') no-repeat; |
There was a problem hiding this comment.
the usual way that neuroglancer main handles svgs is pulling them from https://ikonate.com/, for example, for this help icon, it would be "ikonate/icons/help.svg?raw".
for this svgs I think it would be great to either remove the svgs and stick with the styling in the css (for example, the checkboxes look well still if we remove the svgs). For things like the tooltips, a strategy like this would be good:
- Define the path in typescript, and make an icon using it.
import svg_help from "ikonate/icons/help.svg?raw";
import { makeIcon } from "#src/widget/icon.js";
const generalSettingsTooltip = makeIcon({ svg: svg_help });
// same for other tooltips- In the CSS define the stroke color via:
.neuroglancer-screenshot-tooltip svg * {
stroke: black;
}or similar / more specific for different tooltips if needed.
src/ui/screenshot_menu.css
Outdated
| .neuroglancer-screenshot-tooltip { | ||
| cursor: pointer; | ||
| } | ||
| .neuroglancer-screenshot-dialog .neuroglancer-screenshot-close-and-help .neuroglancer-screenshot-tooltip { |
There was a problem hiding this comment.
it think it would be a nice change in the typescript to give this a class or similar identifier like
neuroglancer-screenshot-general-tooltip and then use this for the declaration in the css
|
Just a quick note we would like to swap the order of "Physical resolution" and "pixel resolution" in the layer table, so "physical resolution" would be the last column |
