-
Notifications
You must be signed in to change notification settings - Fork 34
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
ui: add dropdowns for filtering shared workflows #371
ui: add dropdowns for filtering shared workflows #371
Conversation
6222cd9
to
85db3fd
Compare
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.
Good job! Left some comments, mainly cosmetics and about the user fetch actions.
@@ -2,7 +2,7 @@ | |||
-*- coding: utf-8 -*- | |||
|
|||
This file is part of REANA. | |||
Copyright (C) 2020, 2021, 2022 CERN. | |||
Copyright (C) 2020, 2021, 2022, 2023 CERN. |
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.
We should update the copyright notice in all these files to include 2024!
.then((resp) => { | ||
dispatch({ | ||
type: USERS_SHARED_WITH_YOU_RECEIVED, | ||
users_shared_with_you: resp.data.users_shared_with_you, |
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.
users_shared_with_you: resp.data.users_shared_with_you, | |
usersSharedWithYou: resp.data.users_shared_with_you, |
Switching to camelCase to follow JS convention like done elsewhere in the file, do you agree?
.then((resp) => { | ||
dispatch({ | ||
type: USERS_YOU_SHARED_WITH_RECEIVED, | ||
users_you_shared_with: resp.data.users_you_shared_with, |
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.
Same as above
|
||
export function fetchUsersSharedWithYou() { | ||
return async (dispatch) => { | ||
dispatch({ type: USERS_SHARED_WITH_YOU_FETCH }); |
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.
Why do we need to dispatch this action (same thing below, in fetchUsersYouSharedWith
) if it does not result in a change of the state? I think we can maybe delete this type of action altogether.
const sharing = (state = sharingInitialState, action) => { | ||
switch (action.type) { | ||
case USERS_SHARED_WITH_YOU_FETCH: | ||
return { ...state }; |
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.
Related to the comment above in actions.js
. Since the action does not modify the state, maybe we can just delete it (same for USERS_YOU_SHARED_WITH_FETCH
).
WDYT?
@@ -133,7 +141,7 @@ function Workflows() { | |||
|
|||
return ( | |||
<div className={styles.container}> | |||
<Container text> | |||
<Container id={styles["workflow-list-container"]}> |
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.
Very minor: in the rest of the codebase, it looks to me that classes are always used instead of IDs when used for styling purposes. While it's by no means a hard rule, what do you think about switching to className
just for consistency?
/* Mobile */ | ||
@media only screen and (max-width: 767px) { | ||
#workflow-list-container { | ||
width: 500px; | ||
} | ||
} | ||
|
||
/* Tablet */ | ||
@media only screen and (min-width: 768px) and (max-width: 991px) { | ||
#workflow-list-container { | ||
width: 700px; | ||
} | ||
} | ||
|
||
/* Small Monitor */ | ||
@media only screen and (min-width: 992px) and (max-width: 1199px) { | ||
#workflow-list-container { | ||
width: 800px; | ||
} | ||
} | ||
|
||
/* Large Monitor */ | ||
@media only screen and (min-width: 1200px) { | ||
#workflow-list-container { | ||
width: 825px; | ||
} | ||
} |
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.
An idea: I was thinking about it, and given that SemanticUI already handles the different breakpoints for the different screen sizes well, maybe we could turn workflow-list-container
into a grid columns and handle everything on the JS side. After all (I'm quoting from the Semantic UI docs) "A text container is a simpler markup alternative to using a grid with a single column". In this way we would only rely on the Semantic UI breakpoint classification and the code would be a bit cleaner. WDYT?
#sharing-filter-dropdown:hover, | ||
#sharing-filter-dropdown:focus, | ||
#sharing-filter-dropdown:active { | ||
z-index: 2; |
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.
Why is this needed?
#selected-user-dropdown.input { | ||
border-top-left-radius: 0%; | ||
border-bottom-left-radius: 0%; | ||
margin-left: -1px; | ||
} |
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 think we can remove these lines, correct?
height: 14+2px; | ||
margin-bottom: -2px; |
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 think we can remove these lines, correct?
Included in #375 |
Adds the dropdowns for filtering shared workflows like so:
The width of the workflow list has been increased so that the dropdowns could fit on the same row as the existing filters/sorters.
Closes #367