-
Notifications
You must be signed in to change notification settings - Fork 340
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
Optimize Tagline Form page #972
Optimize Tagline Form page #972
Conversation
1091e10
to
e21cee6
Compare
siteRes: GetSiteResponse; | ||
siteForm: EditSite; | ||
loading: boolean; | ||
editingRow: number | null; |
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.
Use undefined:
editingRow?: number
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.
Changed
<table id="taglines_table" className="table table-sm table-hover"> | ||
<thead className="pointer"> | ||
<th></th> | ||
<th style="width:121px"></th> |
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.
Don't hard code widths like this. Use responsive bootstrap tables.
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 is pulled from emoji table, used for same reason, just to give a min width to the column to have two buttons on the same row.
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.
Okay, but I highly suggest you read over how bootstrap does responsive forms: https://getbootstrap.com/docs/5.0/forms/overview
This work moves the tagline editing from the main Site Settings form into its own tab, like emojis. It also is optimized for larger lists, allowing hundreds of taglines without slowing down the page.
I wrote this up pretty quick, didn't want to refactor too much, just needed to get past a blocker for hexbear. Its still worthy of upstreaming it, though, as current implementation falls apart if you have lots of taglines.