-
-
Notifications
You must be signed in to change notification settings - Fork 323
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
Modals and Forms Visual Updates #3798
base: develop
Are you sure you want to change the base?
Conversation
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.
Looks good to me so far! I added a couple small comments.
@@ -190,7 +194,7 @@ | |||
onProceed={handleSave} | |||
onCancel={onClose} | |||
size="small" | |||
proceedButton={{ label: $_('add') }} | |||
proceedButton={{ label: $_('Add Foreign Key Constraint') }} |
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.
i18n keys should be in snake_case
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 will update, thanks.
@@ -300,10 +300,10 @@ | |||
text={targetTableName | |||
? $_('columns_removed_from_table_added_to_target', { | |||
values: { count: $columns.length }, | |||
}) | |||
}) + ' A link column will be created to maintain the relationship between records.' |
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.
When you finish putting all your strings into dict.json, make sure you don't have any post-translation string concatenation like this.
mathesar_ui/src/App.svelte
Outdated
p { | ||
line-height: 1.5; | ||
} |
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.
This seems a little odd to me. I'm fine with changing the line height. But I wouldn't expect us to customize it specifically for paragraphs. We don't even use <p>
very often in Mathesar. So I'm worried that this CSS will lead to inconsistent style.
Can you explain why you set this here?
I'd typically expect to see line-height
set on a very high-level element like html
and then for it to sometimes be overridden for elements like headings.
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.
The blocks of text were too tight and I was concerned about making this a global change in case it affected buttons or other elements like the pills. I will consider your feedback and try adjusting it globally.
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 only looked at the PR description, not the code.)
This looks great @ghislaineguerin, very nice work! Just one small piece of feedback.
mathesar_ui/src/systems/table-view/constraints/NewUniqueConstraint.svelte
Outdated
Show resolved
Hide resolved
mathesar_ui/src/systems/table-view/link-table/LinkTableForm.svelte
Outdated
Show resolved
Hide resolved
mathesar_ui/src/systems/table-view/link-table/LinkTableForm.svelte
Outdated
Show resolved
Hide resolved
mathesar_ui/src/systems/table-view/link-table/LinkTableForm.svelte
Outdated
Show resolved
Hide resolved
mathesar_ui/src/systems/table-view/link-table/LinkTableForm.svelte
Outdated
Show resolved
Hide resolved
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've not thoroughly review the changes from a stylistic or aesthetic perspective because I don't expect to have strong opinions about that and I trust Ghislaine's judgement here.
But I did add some critique on the code pointing out some i18n issues.
@seancolsen |
@ghislaineguerin cool. Is this ready for re-review then? |
Has my review comment also been addressed? |
@seancolsen Yes, it is ready now. @kgodey Yes, It is solved because I kept the inline layout for the new column input. I've moved away from using the help descriptions inside outcomes as it fractures the outcome text. The label is now clearly differentiated from the content. See below. |
Looks good @ghislaineguerin but are we losing the colors for the table names? |
@kgodey we only had colors in the context for the Create Link dialog. |
Ah right, sorry @ghislaineguerin, wasn't paying enough attention. |
Changes
Modals Update
I've been improving modals across Mathesar to ensure consistency, better usability, and adherence to design guidelines. Here's a summary of the changes and improvements:
Alignment and Spacing Issues
Styling and Contrast Issues
Consistency Enhancements
Create Link Updates
The "Create Link" modal has been revised to enhance clarity and maintain consistency with other forms in the app.
Guidelines
https://hackmd.io/0MNSBOSoRUC0cnYEqMj9eA?edit