-
-
Notifications
You must be signed in to change notification settings - Fork 941
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
SAK-50454 Polls improve voting ui responsiveness #12856
Conversation
|
||
<p class="act"> | ||
<p class="act" style="flex-wrap:nowrap;"> |
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.
prefer not using inline style
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.
Thanks! I updated the code with a bootstrap class instead.
@@ -108,9 +109,9 @@ <h1 rsf:id="poll-list-title">Polls</h1> | |||
</td> | |||
</tr> | |||
</tbody> | |||
</table> | |||
</table></div> |
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.
Nice work! Just a quick suggestion: placing </table>
and </div>
on separate lines improves readability and maintainability. Could you adjust the formatting?
Removed redundant javascript script types Indented all the markup properly We need to remove those vula references at some point :)
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 added another commit. Don't be afraid to remove redundant code, like the javascript script type on script tags. If I'm working in a file and I see old stuff that might not actually be related to the thing I'm fixing, I will still clean it up. That way our codebase steadily improves. Don't go crazy and start looking at the entire polls tool, but changes to the area where you're working are fine.
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 overall! However, I noticed there's no i18n support, @adrianfish Is it okay?
@Sharadhi98 , please test one last time with the latest changes, and once everything looks good, we can merge. Thanks!
</div> | ||
</body> | ||
</html> | ||
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> |
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.
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> | |
<!DOCTYPE html> |
For some reason, I'm unable to test the Polls tool. Even though there's are no compilation errors, I get an error when I load the tool on my local. Here's a screenshot. |
Co-authored-by: Kunal Jaykam <[email protected]>
@Sharadhi98 I'll pull your branch again and test things out. If it's all good, I'll approve it and merge. |
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.
Builds and works fine. Thanks Sharadhi.
I have resolved the responsiveness issue of the polls tool by using table-responsive. I observed that the grid system applied to the portal-main-container was interfering with the styling of table-responsive, so I replaced it with flex and added two classes: d-flex and flex-column.
I'd love some feedback on changing the portal-main-container from grid to flex.
Thanks!