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

SAK-51228 Dashboard improve ui responsiveness and layout across widgets and dropdowns #13517

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

st-manu
Copy link
Contributor

@st-manu st-manu commented Mar 31, 2025

https://sakaiproject.atlassian.net/browse/SAK-51228

Dashboard: style fixes, invisible buttons in dark mode, and element overflow

Fix element overflows and column widths in the mobile view of the dashboard. Additionally, the reorder buttons are not visible in dark mode.

Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

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

There are accessibility issues in these changes.

@@ -302,7 +302,7 @@ export class SakaiTasks extends SakaiPageableElement {

<div id="controls">
<div id="filter">
<select @change=${this.filterChanged} .value=${this._currentFilter}>
<select class="w-100 mb-1" @change=${this.filterChanged} .value=${this._currentFilter}>
Copy link

Choose a reason for hiding this comment

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

Looks like this element is missing an accessible name or label. That makes it hard for people using screen readers or voice control to use the control.

@@ -315,7 +315,7 @@ export class SakaiTasks extends SakaiPageableElement {
</select>
</div>
<div id="sort">
<select @change=${this.sortChanged}>
<select class="w-100 mb-3" @change=${this.sortChanged}>
Copy link

Choose a reason for hiding this comment

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

Looks like this element is missing an accessible name or label. That makes it hard for people using screen readers or voice control to use the control.

@st-manu st-manu marked this pull request as draft March 31, 2025 09:53
@ern
Copy link
Contributor

ern commented Mar 31, 2025

@st-manu please provide a better subject line for the commit.
(imagine if every commit was "several fixes and improvements", which they are!)

@ern ern changed the title SAK-51228 Dashboard: several fixes and improvements SAK-51228 Dashboard improve ui responsiveness and layout across widgets and dropdowns Mar 31, 2025
@st-manu
Copy link
Contributor Author

st-manu commented Apr 1, 2025

@ern good point. I'll change it and split it into sub Jiras

@st-manu st-manu force-pushed the dashboard branch 2 times, most recently from 94fe5df to 3efc290 Compare April 1, 2025 07:10
@st-manu st-manu marked this pull request as ready for review April 1, 2025 07:10
@st-manu
Copy link
Contributor Author

st-manu commented Apr 3, 2025

@ern done ✅

Copy link
Contributor

@adrianfish adrianfish left a comment

Choose a reason for hiding this comment

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

You don't need to use parts for the site picker, imo. No need to reach into the component like that.

margin-bottom: 12px;
#site-filter sakai-site-picker::part(select) {
width: 100% !important;
margin-bottom: 0.25rem !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why you're adding a margin inside the site picker component. You could just add it to the div#filter element as a top margin.

margin-bottom: 12px;
#site-filter sakai-site-picker::part(select) {
width: 100% !important;
margin-bottom: 1rem !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, add this to the surrounding div, not the component.

#site-filter {
margin-bottom: 12px;
#site-filter sakai-site-picker::part(select) {
width: 100% !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should really go in the component itself. Add a styles block. We don't need to expose the part just for these simple stylings. Also, why is it marked as !important? What is overriding it?

@st-manu
Copy link
Contributor Author

st-manu commented Apr 7, 2025

@adrianfish I can add the margin to the div, but I need to control the width of the selector, which can't be done by just modifying the width of the div, it doesn't affect the selector at all. As it is now, for sites with long titles (which, as we've discussed, is typical in Spanish universities), the selector overflows off the screen. The current design works fine for the nightly test sites, but outside of those cases, the dashboard has poor responsive behavior in this regard.

I brought up the selector because I’m not sure to what extent I can change its width to make it 100% in all the places where it’s used (I don’t know how many parts of Sakai use it). For the dashboard widgets, it significantly improves the interface, but it might break things elsewhere if changes aren't made accordingly.

I'll change the !important part later. Can you confirm if I should move the width: 100% inside the selector component?

@adrianfish
Copy link
Contributor

@st-manu I see your logic. It makes sense to not make the actual component so opinionated. Thanks for the explanation :)

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.

3 participants