closes #2288: rework workspace type selection dialog #2352

glefi · 2019-04-01T13:41:04Z

@ggeisler The ListItemText component provides the possibility to define a primary and a secondary text, that I used. I'm not sure, if the resulting text formatting matches the desired one.
@jvine Could you attach the workspace type thumbs? I've added some placeholders for now.

ggeisler · 2019-04-01T16:03:51Z

@glefi Thanks for checking. As I mentioned in #2288, the typography for those elements should be styled as an H3 (the label of the workspace type; but leave the element as is; don't make it an <h3 />) and Body1 (the text description of each type). See the typography details for those ("Headline 3" and "Body 1") here: #2093

If you update those elements to match the typographic specifications in #2093, plus update the dialog header to be an H2, the result should look similar to this:

By the way, the mouse selection looks good (highlighting and cursor) but I notice I can't use the dialog with the keyboard. But I know this is WIP so you might not have gotten to that yet.

codecov-io · 2019-04-02T12:39:08Z

Codecov Report

Merging #2352 into master will decrease coverage by 0.21%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master    #2352      +/-   ##
==========================================
- Coverage   92.71%   92.49%   -0.22%     
==========================================
  Files         135      137       +2     
  Lines        1852     1852              
==========================================
- Hits         1717     1713       -4     
- Misses        135      139       +4

Impacted Files	Coverage Δ
src/components/icons/WorkspaceTypeMosaicIcon.js	`0% <0%> (ø)`
src/lib/ListKeyboardNavigation.js	`94.73% <0%> (ø)`	⬆️
src/components/icons/WorkspaceTypeElasticIcon.js	`0% <0%> (ø)`
src/components/WindowSideBarButtons.js	`97.56% <100%> (-0.12%)`	⬇️
src/components/WorkspaceSelectionDialog.js	`66.66% <100%> (-22.23%)`	⬇️

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 9e408be...a50ad21. Read the comment docs.

glefi · 2019-04-02T12:50:51Z

@ggeisler I've changed the text formatting according to the typographic specifications and the keyboard navigation works now, too. But now the required height of the text elements is slightly higher than 100px, maybe we should increase the image height, what do you think?

glefi · 2019-04-02T12:54:44Z

src/components/WorkspaceSelectionDialog.js

+        container={container}
+        id="workspace-settings"
+        onClose={handleClose}
+        onKeyDown={event => this.keyDownHandler(event)}


I don't know if it's common, to set the keyDownListener to the hosting node, but I thought, especially when there will be more workspaceTypes perhaps in the future, we could save some locs this way.

glefi · 2019-04-02T12:56:45Z

src/components/WorkspaceSelectionDialog.js

+  }
+
+  /** */
+  selectPreviousItem() {


A simple toggling function would have been sufficient here, but I tried to be prepared for some additional workspaceTypes that may come in the future.

ggeisler · 2019-04-02T17:06:44Z

But now the required height of the text elements is slightly higher than 100px, maybe we should increase the image height, what do you think?

@glefi Sure, assuming you only need to make the images a little bit taller (110 or 120px?) that sounds fine.

This is looking good but a couple things I noticed (in case you aren't aware):

When I switch to elastic mode (using the mouse) and then choose the workspace type menu item again, the dialog shows the selected mode is Mosaic, when it should reflect that I am currently in Elastic:

This actually works correctly when using the keyboard (nice work on getting the keyboard nav working!) but compare the difference when using the mouse.

The color of the text for the workspace type description <p />s is a gray (I think coming from .jss381 or the simple dialog default styles) but it should be our default color (I think the color in .jss357 that is overriden by .jss381). Otherwise, the color contrast when the item is selected is not accessible.

ggeisler

@glefi This is getting closer but I think we need further work to get it to look right:

Images

It's great to see the SVGs in there but something isn't right about the sizing. These SVGs have a native aspect ratio of 4:3 (e.g., 160px wide by 120px high) but what I see in the app is almost square (from the web inspector it seems that they are sized 126px wide by 120px high), which distorts the image. It looks like you have defined the width and height to be 160x120 but for some reason that is not what is being rendered.

I know we previously discussed making the images taller, but aside from the aspect ratio problem, I think the images and entire rows for the two options are bigger than they need to be. So I suggest:

Make the images 90px in height
Make sure the images have the correct aspect ratio of 4:3

Option heading typography

The "Elastic" and "Mosaic" headings are using an H2 element. In the ticket specifications for this I suggested the headings be:

styled using the H3 typography, but don't use a heading element

I think we should stick to that because these are not structural elements that we want to appear in a list of headings read by a screenreader, for example. So we want them to use a non-heading element (I think a p would work) but then apply the H3 typography styling to them so they appear visually similar to an H3. I don't know how we do that, but I think others on the team have applied a typographical style to change an element from its default typographical styling.

ggeisler

Nice @glefi, the SVGs look great now. I have three more suggestions, which are minor spacing-related suggestions. It looks good overall but I think the spacing should be a bit tighter. I assume you can make these adjustments just to this dialog and these won't affect anything more globally, but let me know if that is not the case.

The first two suggestions are to reduce the vertical padding on the div that contains the option title and description text. They are both to the .jss514 class as shown in the screenshot below.

Add 0 for the top/bottom padding of .jss514.
Reduce the .jss514:last-child padding-bottom to 12px (from 24px)

The other suggestion is to add a new padding-bottom: 0; to .jss328, the p that contains the option titles. That will just provide a bit of vertical space between the option title and its description text and better distinguish the option title from the description.

glefi · 2019-04-12T08:18:56Z

@ggeisler Thanks for your feedback. I've added your suggestions to the pr. What do you think about it?

ggeisler · 2019-04-12T15:02:32Z

Looks good @glefi!

mejackreed · 2019-04-12T15:12:03Z

src/components/WorkspaceSelectionDialog.js

+              <Card className={classes.card}>
+                <CardMedia
+                  className={classes.media}
+                  image="/src/images/icon_workspace_elastic.svg"


I'm concerned that referencing the image this way will hardcode the image path (and its not included in the build). Is there a way to reference the SVG directly?

@mejackreed Do we have a pattern established for this anywhere else in the repo?

Yeah, I think the predominate pattern is to wrap the SVG in a component like we do with the icons:

https://github.com/ProjectMirador/mirador/blob/master/src/components/icons/BookViewIcon.js

@ggeisler Could you provide the two SVGs in a scaled variant (90x120)? Because resizing the files with inkscape seems not to work properly.

@glefi I've updated the gists I previously gave you, so try those and see how they work:

Mosaic: https://gist.github.com/ggeisler/21c514a51ae9eb0cf62288a1f05e6ca0

Elastic: https://gist.github.com/ggeisler/3bacfde8f287748603a6bc33e0ef8197

…dialog

…e key pressed

mejackreed

Looks good @glefi .. thanks for getting this fixed up!

mejackreed · 2019-04-16T14:28:51Z

src/components/WorkspaceSelectionDialog.js

    } = this.props;
    return (
      <Dialog
        aria-labelledby="workspace-selection-dialog-title"
        container={container}
-        id="workspace-settings"
+        id="workspace-selection-dialog"


I like how you renamed this to be more accurate

glefi force-pushed the 2288-update-the-select-workspace-type-dialog branch from 95ae7a4 to 837a98b Compare April 2, 2019 12:34

glefi changed the title ~~[WIP] #2287: rework workspace type selection dialog~~ closes #2287: rework workspace type selection dialog Apr 2, 2019

glefi commented Apr 2, 2019

View reviewed changes

glefi changed the title ~~closes #2287: rework workspace type selection dialog~~ closes #2288: rework workspace type selection dialog Apr 3, 2019

glefi changed the title ~~closes #2288: rework workspace type selection dialog~~ [WIP] #2288: rework workspace type selection dialog Apr 3, 2019

glefi force-pushed the 2288-update-the-select-workspace-type-dialog branch from 514bd95 to 99947ef Compare April 8, 2019 10:32

glefi changed the title ~~[WIP] #2288: rework workspace type selection dialog~~ closes #2288: rework workspace type selection dialog Apr 8, 2019

camillevilla requested a review from ggeisler April 10, 2019 15:05

ggeisler requested changes Apr 10, 2019

View reviewed changes

glefi force-pushed the 2288-update-the-select-workspace-type-dialog branch from 153f472 to dd2ce46 Compare April 11, 2019 07:46

glefi requested a review from ggeisler April 11, 2019 07:52

ggeisler requested changes Apr 11, 2019

View reviewed changes

glefi force-pushed the 2288-update-the-select-workspace-type-dialog branch from dd2ce46 to b2239ef Compare April 12, 2019 08:12

glefi mentioned this pull request Apr 12, 2019

Non-svg images are not being loaded in a plugin #2458

Closed

ggeisler previously approved these changes Apr 12, 2019

View reviewed changes

mejackreed reviewed Apr 12, 2019

View reviewed changes

glefi dismissed ggeisler’s stale review via 47f9cd6 April 15, 2019 10:02

glefi added 6 commits April 16, 2019 15:07

#2288: adds keyboard navigation to WorkspaceSelectionDialog, adds tests

3c70674

#2288: sets the title's typography to h2 for the workspace selection …

7d52d24

…dialog

fix build

b874d31

#2288: increases height of workspace images, adds behaviour for escap…

56b3cc4

…e key pressed

#2288: use ListKeyboardNavigation

25ec581

#2288: removes obsolete tests

48500f0

glefi added 13 commits April 16, 2019 15:07

2288: adds onClose for workspace selection dialog

1bc48a3

updates workspace selection dialog id

721ec65

removes placeholder images

94ec1db

#2288: changes typography and image dimensions

c21d825

cosmetics

d4f3c66

#2288: adds some css

8f4b90d

cosmetics

7c127b6

#2288: adds using of url-loader fir svg

cf46d2a

#2288: wraps workspace type svgs in a component

ee50dfd

#2288: removes obsolete imports

e758836

#2288: updates svgs

2b7d548

cosmetics

57fc0d9

fix build

a50ad21

glefi force-pushed the 2288-update-the-select-workspace-type-dialog branch from 6296487 to a50ad21 Compare April 16, 2019 13:07

mejackreed approved these changes Apr 16, 2019

View reviewed changes

mejackreed merged commit 8ec919d into master Apr 16, 2019

mejackreed deleted the 2288-update-the-select-workspace-type-dialog branch April 16, 2019 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

closes #2288: rework workspace type selection dialog #2352

closes #2288: rework workspace type selection dialog #2352

glefi commented Apr 1, 2019 •

edited

Loading

ggeisler commented Apr 1, 2019

codecov-io commented Apr 2, 2019 •

edited

Loading

glefi commented Apr 2, 2019

glefi Apr 2, 2019

glefi Apr 2, 2019

ggeisler commented Apr 2, 2019

ggeisler left a comment

ggeisler left a comment

glefi commented Apr 12, 2019

ggeisler commented Apr 12, 2019

mejackreed Apr 12, 2019

camillevilla Apr 12, 2019

mejackreed Apr 12, 2019

glefi Apr 15, 2019

ggeisler Apr 15, 2019

mejackreed left a comment

mejackreed Apr 16, 2019

closes #2288: rework workspace type selection dialog #2352

closes #2288: rework workspace type selection dialog #2352

Conversation

glefi commented Apr 1, 2019 • edited Loading

ggeisler commented Apr 1, 2019

codecov-io commented Apr 2, 2019 • edited Loading

Codecov Report

glefi commented Apr 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggeisler commented Apr 2, 2019

ggeisler left a comment

Choose a reason for hiding this comment

Images

Option heading typography

ggeisler left a comment

Choose a reason for hiding this comment

glefi commented Apr 12, 2019

ggeisler commented Apr 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mejackreed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glefi commented Apr 1, 2019 •

edited

Loading

codecov-io commented Apr 2, 2019 •

edited

Loading