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

Add image annotation insert/edit to mirador-annotations #50

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

MImranAsghar
Copy link

  • This image insert/edit input dialog is needed to extend the annotation support to adding image annotation.
  • These image annotation insert/edit changes attempt to implement image insert/edit workflow similar to Mirador 2 for the image annotation. For example, once a url is entered then clicking away from the url field will update the width and height inputs with the image width and height that the url corresponds to.
  • The other part of image annotations would be the image annotation card (list of image annotations displayed on a card on hovered annotations) which I am currently working on that can be added as a separate plugin (to be used with this one) or to mirador 3 eventually

@ggeisler
Copy link

ggeisler commented Apr 6, 2021

@MImranAsghar Thanks for working on this. I have some comments that are strictly about the visual design; I will leave it to others to weigh in on the more technical aspects (though I might have thoughts on the overall user experience as you get additional parts of the feature completed).

I realize that you are probably more focused on the functionality of this feature than the visual design at this point (understandably) but I want to make sure contributors are conscious of following existing Mirador 3 patterns when creating new UI elements. Ideally we'd have some form of detailed guide for Mirador 3 visual design, which would make it easier for developers. We already have divergence in some of the other Mirador 3 plugins so lacking a detailed style guide I want to point out these things so we can minimize further divergence.

Screen Shot 2021-04-06 at 10 49 46 AM

  • A: This doesn't look like the correct header for a modal. See the Workspace options (... menu in Workspace sidebar) and look at either the Import workspace or Export workspace modals for an example of the model I think we should follow. The heading should be larger and use a different font weight variant, I believe.
  • A: Also, we are using sentence case for UI labels in Mirador 3, so this should be "Insert image" (lowercase "image" and I think we could drop the "Edit" part since if the user edits an existing image annotation, they will presumably see the fields filled out and it'll obvious they are editing).
  • B: Since these are headings, not field labels, I believe they should be larger and darker than the placeholder text in the input field. I'm not sure we have a good example of this pattern yet, but I'd suggest using a heading similar to what you are currently using for the "Insert/Edit Image" header for these headings.
  • B: Not a big deal, but I think we could add the word "Image" to both of these labels for a bit more clarity: "Image source" and "Image dimensions"
  • C: "Url" should be in all caps: "URL"
  • D: The checkbox and label of Constrain proportions should have more horizontal space between it and the Height input. Currently the checkbox is closer to the Height input than the Width and Height inputs are to each other. The inputs are associated with each other so visually they should appear as a group, and the Constrain proportions checkbox and label should visually appear as a separate (though related) control, which means there needs to be at least ~20 or 30px between the Height input and the checkbox.
  • E: "Proportions" should be lowercase "proportions"
  • F: See the Import workspace or Export workspace modals for correct styling of these buttons. "Add" should be represented as the primary action, and the colors and hover effects of the buttons should be different. Hopefully you can fairly easily just use the styles applied to the Import workspace or Export workspace modal buttons and get the correct styling.

@ggeisler
Copy link

ggeisler commented Apr 6, 2021

@MImranAsghar Thanks for working on this. I have some comments that are strictly about the visual design; I will leave it to others to weigh in on the more technical aspects (though I might have thoughts on the overall user experience as you get additional parts of the feature completed).

I realize that you are probably more focused on the functionality of this feature than the visual design at this point (understandably) but I want to make sure contributors are conscious of following existing Mirador 3 patterns when creating new UI elements. Ideally we'd have some form of detailed guide for Mirador 3 visual design, which would make it easier for developers. We already have divergence in some of the other Mirador 3 plugins so lacking a detailed style guide I want to point out these things so we can minimize further divergence.

Screen Shot 2021-04-06 at 10 49 46 AM

  • A: This doesn't look like the correct header for a modal. See the Workspace options (... menu in Workspace sidebar) and look at either the Import workspace or Export workspace modals for an example of the model I think we should follow. The heading should be larger and use a different font weight variant, I believe.
  • A: Also, we are using sentence case for UI labels in Mirador 3, so this should be "Insert image" (lowercase "image" and I think we could drop the "Edit" part since if the user edits an existing image annotation, they will presumably see the fields filled out and it'll obvious they are editing).
  • B: Since these are headings, not field labels, I believe they should be larger and darker than the placeholder text in the input field. I'm not sure we have a good example of this pattern yet, but I'd suggest using MuiTypography-h3 for these headings.
  • B: Not a big deal, but I think we could add the word "Image" to both of these labels for a bit more clarity: "Image source" and "Image dimensions"
  • C: "Url" should be in all caps: "URL"
  • D: The checkbox and label of Constrain proportions should have more horizontal space between it and the Height input. Currently the checkbox is closer to the Height input than the Width and Height inputs are to each other. The inputs are associated with each other so visually they should appear as a group, and the Constrain proportions checkbox and label should visually appear as a separate (though related) control, which means there needs to be at least ~20 or 30px between the Height input and the checkbox.
  • E: "Proportions" should be lowercase "proportions"
  • F: See the Import workspace or Export workspace modals for correct styling of these buttons. "Add" should be represented as the primary action, and the colors and hover effects of the buttons should be different. Hopefully you can fairly easily just use the styles applied to the Import workspace or Export workspace modal buttons and get the correct styling.

@MImranAsghar
Copy link
Author

Hi, Thanks for pointing these out. All your comments look very useful, I will start looking into these one by one and make the required changes 👍

- This dialog is needed to extend the annotation support to adding image annotation.
- Expects user to enter url; calculates width and height of image
- Includes image dialog input validation; checks url, width, height
... in the image annotation input/edit

- If the constrain proportions checkbox is checked in image annotation input, this functionality will make sure that the image proportions are maintained if either a new width or height is entered
- These image annotation insert/edit changes attempt to implement image insert/edit workflow similar to Mirador 2
@MImranAsghar MImranAsghar force-pushed the Add-img-annotation-insert-edit branch from 66071e3 to 1cf3ea4 Compare May 13, 2021 15:01
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.

2 participants