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

New dashboard #3150

Merged
merged 34 commits into from
Nov 11, 2022
Merged

New dashboard #3150

merged 34 commits into from
Nov 11, 2022

Conversation

carlobeltrame
Copy link
Member

@carlobeltrame carlobeltrame commented Nov 8, 2022

Supersedes #3053

The chip colors and avatar colors unfortunately don't work on devel... In fact, the category chips have vanished completely. Maybe the colorjs.io package isn't compatible with our build process either? Oh well, we can fix that in a separate PR. #3154

Ideas for optional further refinement (let me know what you'd like me to implement in this PR):

  • Skeleton loaders -> done
  • Auto-select the next upcoming period, if any
  • Persist the filters in the URL, for sharing filtered lists and so that when navigating to an activity and back, the same filters are restored
  • [your idea here]

@carlobeltrame carlobeltrame added the deploy! Creates a feature branch deployment for this PR label Nov 8, 2022
Copy link
Member

@manuelmeister manuelmeister left a comment

Choose a reason for hiding this comment

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

Cool, thank you!

Yes, I noticed that too on dev deployment. (Not just chips, but also the avatar). A fix for the library is awaiting a release. But does it work locally with you?

Th url queries would be nice, but could be done in a next PR.

frontend/src/components/dashboard/ActivityRow.vue Outdated Show resolved Hide resolved
</template>

<script>
import AvatarRow from './AvatarRow.vue'
Copy link
Member

Choose a reason for hiding this comment

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

When do we relative import components and when via the @ notation?

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 do so more or less at random. I try to avoid too many ../../../../, and especially try to import from the symlinked common instead of the common folder outside the frontend folder. But most of the time I let phpstorm manage this

Copy link
Member

Choose a reason for hiding this comment

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

Just my 2 cents:

  • Relative paths make refactoring easy, if the components are tightly coupled (mostly for components in the same folder or subfolders). In this example I could rename or move the dashboard folder, and things just work (1 path only to fix).
  • For anything else (usage of generic components, base components, etc.) I normally use @ because it's easier to search & replace in case of refactoring. Any ../../../../ thing is super difficult to catch and relies a lot on IDE magic for refactoring.

frontend/src/components/dashboard/SelectFilter.vue Outdated Show resolved Hide resolved
Copy link
Member

@usu usu left a comment

Choose a reason for hiding this comment

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

Network calls can still be improved.

  • Loading the page directly --> each activity is loaded with a separate network call
  • Navigating to the page from /camps --> each day and each camp_collaboration is loaded with a separate network call

common/locales/en.json Outdated Show resolved Hide resolved
<span class="e-subtitle">{{ duration }}</span>
</td>
<td style="width: 100%" class="contentrow">
<router-link :to="routerLink" class="text-decoration-none black--text">
Copy link
Member

Choose a reason for hiding this comment

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

I'd vote to keep the text-decoration here. It's visually not easy to see that titles are links but the rest not.
And it would make it consistent with "story" view (activity titles also links). Not mission critical, so can also be discussed/come in separate improvements.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how to handle this yet generally. I would like to develop a concept for such links #3153. Just on GitHub they have different ways as well, so I'm not sure if everything needs to be the same.
I think for the moment we can leave this as is.

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 think the links are a little hidden right now as well. But adding all the underlines makes the view very nervous. Ideally, I'd like the whole row to be clickable, but that's not so easy with table rows to do in a semantically correct way last time I checked.
As a compromise, I added the underline only on hover. Not much help on mobile though.

frontend/src/views/camp/Dashboard.vue Outdated Show resolved Hide resolved
frontend/src/views/camp/Dashboard.vue Outdated Show resolved Hide resolved
frontend/src/views/camp/Dashboard.vue Outdated Show resolved Hide resolved
frontend/src/locales/en.json Outdated Show resolved Hide resolved
@carlobeltrame
Copy link
Member Author

carlobeltrame commented Nov 9, 2022

@manuelmeister @usu thanks for the good inputs! All your comments should be adressed now. Please check again. In the meantime, did someone already start fixing the colorjs.io issue? There's also sentry issues for it. EDIT: #3154

@carlobeltrame carlobeltrame removed the deploy! Creates a feature branch deployment for this PR label Nov 9, 2022
Copy link
Member

@usu usu left a comment

Choose a reason for hiding this comment

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

Nice, looks good to me for merging

Some potential improvements (next PR):

  • Loading spinner
  • Days are still loaded with individual network requests when navigating to the dashboard from another page

Copy link
Member

@manuelmeister manuelmeister left a comment

Choose a reason for hiding this comment

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

LGTM. The remaining things can be put into an issue

  • I would leave out 0min in the time entry, because it doesn't add any value.
  • Now that it says clear in the menu, I would visually distinguish it from the other options

@manuelmeister manuelmeister mentioned this pull request Nov 11, 2022
5 tasks
Comment on lines 14 to 19
<v-list-item-title class="d-flex">
<span class="flex-grow-1">{{
$tc('components.dashboard.selectFilter.clear')
}}</span>
<v-icon right>mdi-close</v-icon>
</v-list-item-title>
Copy link
Member

@manuelmeister manuelmeister Nov 11, 2022

Choose a reason for hiding this comment

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

Can you make it gray to reduce its importance? In my draft it was black because it was itself an option, now it just resets the filter.

@carlobeltrame carlobeltrame merged commit a83df19 into ecamp:devel Nov 11, 2022
@carlobeltrame carlobeltrame deleted the dashboard branch November 11, 2022 11:31
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