feat: favourite ui feature #8210
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8210 +/- ##
=======================================
Coverage 41.93% 41.93%
=======================================
Files 176 176
Lines 22992 22992
=======================================
Hits 9641 9641
Misses 11966 11966
Partials 1385 1385 Continue to review full report at Codecov.
|
ciiay
left a comment
ciiay
left a comment
There was a problem hiding this comment.
Looks good on the Tiles View. How about List View?
rbreeze
left a comment
rbreeze
left a comment
There was a problem hiding this comment.
One minor comment. This is a great feature!
There was a problem hiding this comment.
Can we right justify the star all the way? The easiest way to accomplish this is to wrap the application title and star button in a flexbox (display: flex), and set margin-left to auto for the star.
There was a problem hiding this comment.
Either right-justified, or a little right-padded next to the application name would be my ask as well.
There was a problem hiding this comment.
Yes, definitely - trying to understand where this routes get added, so that the icons are placed properly
ciiay
left a comment
ciiay
left a comment
There was a problem hiding this comment.
Minor comments on code improvement. And maybe rename the filter with something like "Favourites Only"? Because without the check, it already shows favorites, just mixed with non-favorites.
Btw, Favourites seems British English, maybe we want to keep it US English^^;;?
There was a problem hiding this comment.
favouritesApplist should follow camelcase naming rule, like favouritesAppList.
There was a problem hiding this comment.
Good use of {' '} 👍 , better practice than .
|
Since favorites should be able to be quickly accessed, how about moving the filter "Favorites only" to the top of the filter list? What do people think about this? |
haha, school taught me british english - I am ok with any spelling - what do you suggest @rbreeze , @jannfis |
|
I agree it should probably be "Favorites" to match the American English in the rest of the UI. Also I agree with the change to "Favorites Only" |
saumeya
force-pushed
the
favourite-branch
branch
from
8eeb6bb to
247182a
Compare
|
@rbreeze - which is preferred?
|
|
I prefer the latter with the larger star. |
Signed-off-by: saumeya <[email protected]> lint fix Signed-off-by: saumeya <[email protected]>
|
@saumeya Can you please resolve the merge conflicts? |
Signed-off-by: saumeya <[email protected]>
Signed-off-by: saumeya <[email protected]> use splice instead of filter Signed-off-by: saumeya <[email protected]>
saumeya
force-pushed
the
favourite-branch
branch
from
247182a to
02656bc
Compare
|
alexmt
left a comment
alexmt
left a comment
There was a problem hiding this comment.
Thanks for the thorough review, everyone!
|
@rbreeze can you please approve as well so I can merge ( unless you want to request some changes ) |
* feat: favourite ui feature (#8210) Signed-off-by: saumeya <[email protected]>
* feat: favourite ui feature (argoproj#8210) Signed-off-by: saumeya <[email protected]> Signed-off-by: pashavictorovich <[email protected]>
6 participants
Signed-off-by: saumeya [email protected]
closes #5936
54886c83-7e66-4a2f-9899-f4ec865fce2a.mp4
Signed-off-by: saumeya [email protected]
Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist: