Skip to content

Adopted the new settings dialog component for obo nodes#1944

Merged
FrenjaminBanklin merged 3 commits into
ucfopen:dev/28-jadeitefrom
maufcost:issue/1775-settings-dialog
Feb 8, 2022
Merged

Adopted the new settings dialog component for obo nodes#1944
FrenjaminBanklin merged 3 commits into
ucfopen:dev/28-jadeitefrom
maufcost:issue/1775-settings-dialog

Conversation

@maufcost
Copy link
Copy Markdown
Contributor

@maufcost maufcost commented Jan 6, 2022

Node dialogs that were not updated:

  • text, heading, list, horizontal line, code, question, question bank, html: no settings dialog.
  • math, button, and table: uses a different type of dialog.
  • iframe: not updated because of Updated iframe properties dialog #1828.
  • Materia widget: already updated.

Node dialogs that were updated:

  • figure: both dialogs updated.
  • Youtube: updated.

As always, let me know if you would like me to make any changes.

Fixes #1775

Copy link
Copy Markdown
Contributor

@jpeterson976 jpeterson976 left a comment

Choose a reason for hiding this comment

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

The only thing that I noticed is that the width and height input boxes for custom sized figures are no longer centered vertically with the new dialog, but everything else looks to be the same as the old versions.

@maufcost
Copy link
Copy Markdown
Contributor Author

Good catch! Fixing that now.

jpeterson976
jpeterson976 previously approved these changes Jan 11, 2022
Copy link
Copy Markdown
Contributor

@jpeterson976 jpeterson976 left a comment

Choose a reason for hiding this comment

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

Great, LGTM!

Copy link
Copy Markdown
Contributor

@deundrewilliams deundrewilliams left a comment

Choose a reason for hiding this comment

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

All updated dialogs are centered, lgtm

@FrenjaminBanklin
Copy link
Copy Markdown
Contributor

Everything is functioning properly after the change, so good work there.

The only thing I can see possibly still needing work (unless this is somehow only happening to me) is that the settings windows seem to have way more white space than they need. For example, currently running on production:
Screen Shot 2022-01-27 at 3 06 57 PM
versus:
Screen Shot 2022-01-27 at 3 07 08 PM
and:
Screen Shot 2022-01-27 at 3 08 45 PM
versus:
Screen Shot 2022-01-27 at 3 09 13 PM

If it's a significant burden to tighten these up then I wouldn't necessarily block the merge over it, but if it's possible then I think it would be preferred over the significant extra padding.

…dialogs as close as possible to that of the old dialogs.
@FrenjaminBanklin
Copy link
Copy Markdown
Contributor

@maufcost Double-check that the two minor changes I made to the CSS didn't bug up anything you've done, otherwise this looks like it's ready to go.

@maufcost
Copy link
Copy Markdown
Contributor Author

maufcost commented Feb 1, 2022

Checking that now, and I will get back to you in a couple minutes :)

@maufcost
Copy link
Copy Markdown
Contributor Author

maufcost commented Feb 1, 2022

Just checked all the updated modals. As far as my testing goes, they are good to go!

deundrewilliams
deundrewilliams previously approved these changes Feb 7, 2022
Copy link
Copy Markdown
Contributor

@deundrewilliams deundrewilliams left a comment

Choose a reason for hiding this comment

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

Dialogs are still centered after CSS updates, lgtm

jpeterson976
jpeterson976 previously approved these changes Feb 8, 2022
Copy link
Copy Markdown
Contributor

@jpeterson976 jpeterson976 left a comment

Choose a reason for hiding this comment

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

Stuff still works, LGTM!

@FrenjaminBanklin FrenjaminBanklin changed the base branch from dev/27-pyrite to dev/28-jadeite February 8, 2022 18:29
@FrenjaminBanklin FrenjaminBanklin dismissed stale reviews from jpeterson976 and deundrewilliams February 8, 2022 18:29

The base branch was changed.

@FrenjaminBanklin FrenjaminBanklin merged commit cf64122 into ucfopen:dev/28-jadeite Feb 8, 2022
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