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

[ENG-6191] Improve Styling and Add Total Users Label #2327

Conversation

Johnetordoff
Copy link
Collaborator

@Johnetordoff Johnetordoff commented Sep 13, 2024

  • Ticket: [https://openscience.atlassian.net/browse/ENG-6191]
  • Feature flag: n/a

Purpose

Standardize user tab look and make work for mobile, put in total users label.

Summary of Changes

  • standardize select colors
  • add label for total number of users

Screenshot(s)

Screenshot 2024-09-20 at 2 57 33 PM

Side Effects

QA Notes

@Johnetordoff Johnetordoff changed the base branch from develop to feature/insti-dash-improv September 13, 2024 18:05
@Johnetordoff Johnetordoff force-pushed the improve-mobile-styling-inst-dashboard branch from d4ff7f7 to 6ff2fdf Compare September 13, 2024 18:26
@Johnetordoff Johnetordoff force-pushed the improve-mobile-styling-inst-dashboard branch from 6ff2fdf to ae93ff2 Compare September 13, 2024 18:29
@Johnetordoff Johnetordoff changed the title Improve mobile styling inst dashboard [ENG-6191] Improve Styling and Add Total Users Label Sep 13, 2024
@Johnetordoff Johnetordoff marked this pull request as ready for review September 13, 2024 18:54
Copy link
Contributor

@futa-ikeda futa-ikeda left a comment

Choose a reason for hiding this comment

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

Just a couple minor changes. Could you add a screenshot for mobile view to the PR as well?

<span local-class='total-users-count'>
{{@totalUsers}}
</span>
<label>{{t 'institutions.dashboard.users_list.total_users'}}</label>
Copy link
Contributor

Choose a reason for hiding this comment

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

<label> feels a bit inappropriate to use here, as there is no <input> that this is labelling. Could likely just be another span I think

@@ -142,6 +142,11 @@
margin-bottom: 10px;
}

.dropdown-trigger {
padding: 9px;
color: #337ab7;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
color: #337ab7;
color: $color-select;

…ForOpenScience/ember-osf-web into improve-mobile-styling-inst-dashboard

* 'feature/insti-dash-improv' of https://github.com/CenterForOpenScience/ember-osf-web:
  Removed old tests
  Fixed some api issues
  Added a new test and removed an old one
  Added a new total count kpi

# Conflicts:
#	translations/en-us.yml
@Johnetordoff
Copy link
Collaborator Author

Johnetordoff commented Sep 18, 2024

@Johnetordoff remember to close the hide/add dropdown on clicking apply and add scroll for the table on mobile.

…ForOpenScience/ember-osf-web into improve-mobile-styling-inst-dashboard

* 'feature/insti-dash-improv' of https://github.com/CenterForOpenScience/ember-osf-web:
  Added a test with test-data elements
  Updates to add the project total

# Conflicts:
#	translations/en-us.yml
@Johnetordoff Johnetordoff force-pushed the improve-mobile-styling-inst-dashboard branch 3 times, most recently from bb31bd3 to ae7adb0 Compare September 19, 2024 14:44
@Johnetordoff Johnetordoff force-pushed the improve-mobile-styling-inst-dashboard branch from ae7adb0 to e3e1c76 Compare September 19, 2024 14:55
}

/* Add media query for mobile devices */
@media (max-width: 1000px) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We've tried to move away from hard-coding pixel values into our CSS. Instead, in the template you could use our is-mobile helper

<div local-class=table-wrapper {{if (is-mobile 'mobile'}}>

then change this line to be .mobile and adjust the nested selectors as needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I think this scrolling behavior and no-wrap on the text is something we may want regardless of mobile or not. Let me doublecheck with product though

Comment on lines 281 to 282
.table table {
min-width: 1000px; /* Set the table's width to be wider than the viewport */
Copy link
Contributor

Choose a reason for hiding this comment

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

I worry about setting an explicit min-width here, as the table may not need the full 1000px as users can hide columns and whatnot... Can this be implemented without this hard-coded min-width?

Copy link
Contributor

@futa-ikeda futa-ikeda left a comment

Choose a reason for hiding this comment

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

✨✨

@@ -51,71 +57,73 @@
<Button @type='secondary' {{on 'click' dd.actions.close}}>
{{t 'general.cancel'}}
</Button>
<Button @type='primary' {{on 'click' this.applyColumnSelection}}>
<Button @type='primary' {{on 'click' this.applyColumnSelection}} {{on 'click' dd.actions.close}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for future reference, if you want to have a click even do multiple actions in sequence, you can use either the pipe or queue helper like
{{on 'click' (queue this.firstAction this.secondAction)}}

Docs for those can be found here: https://github.com/DockYard/ember-composable-helpers?tab=readme-ov-file#queue

@Johnetordoff Johnetordoff merged commit 25d34a4 into CenterForOpenScience:feature/insti-dash-improv Sep 20, 2024
9 checks passed
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.

2 participants