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

mvc/view: Ensure fields stay aligned relatively to another when headers are used in forms #8364

Merged
merged 4 commits into from
Feb 28, 2025

Conversation

Monviech
Copy link
Member

@Monviech Monviech commented Feb 20, 2025

This fixes an issue where introducing headers in forms will botch the alignment of fields because they all spawn their own table. Using relative sizes in the table and colgroup fixes this issue.

This also improves responsiveness of the modal.

Bonus: This also improved how form modals render on mobile devices.

@Monviech Monviech added the cleanup Low impact changes label Feb 20, 2025
@Monviech Monviech requested review from fichtner and swhite2 February 20, 2025 14:51
@Monviech Monviech self-assigned this Feb 20, 2025
@Monviech Monviech force-pushed the form-dialog-alignment branch from c8e0f40 to 812175b Compare February 21, 2025 07:56
@fichtner
Copy link
Member

I'm a bit skeptical having dealt with % width whac-a-mole code changes over the last decade :/

… modal-dialog and form-inline exist in the same view (e.g. dnsmasq).
…n msgzone_width is defined (e.g. in Intrusion Detection)
@Monviech
Copy link
Member Author

I would like the opportunity to whack the mole too :D

@Monviech Monviech force-pushed the form-dialog-alignment branch from 9242347 to 431544b Compare February 21, 2025 15:17
@Monviech
Copy link
Member Author

Needed to force push this since I accidentally pushed something else in here that belongs to another branch.

Copy link
Member

@swhite2 swhite2 left a comment

Choose a reason for hiding this comment

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

This does seem to improve modal/form consistency a lot :)

<colgroup>
{% if msgzone_width is defined %}
Copy link
Member

Choose a reason for hiding this comment

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

Do we even need the msgzone? Seems it's only used by IDS

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to keep the change backwards compatible. I dont know the reason behind this in IDS.

Copy link
Member

Choose a reason for hiding this comment

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

these properties should ideally live inside a form metadata tag (or if they're unique cases, handled by javascript), but the scope for that is larger than this change so not worth the effort to look into it here.

Copy link
Member

@swhite2 swhite2 left a comment

Choose a reason for hiding this comment

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

I don't mind this change, although considering @fichtner's reservations best if you sign off on this as well.

Copy link
Member

@fichtner fichtner left a comment

Choose a reason for hiding this comment

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

we'll just have to see I guess

@Monviech
Copy link
Member Author

Let's give this a try then, if there are complains we can always revert it.

@Monviech Monviech merged commit 387c381 into master Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Low impact changes
Development

Successfully merging this pull request may close these issues.

3 participants