Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
task/WG-232-React-Listing-UI #268
base: main
Are you sure you want to change the base?
task/WG-232-React-Listing-UI #268
Changes from all commits
82ac31a
8304f93
b5c2a4e
e5a4817
0ff0f14
d55959d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
this width exceeds its parent when maps have long names or when the browser is narrow. So long map names then exceed the screen width and the
text-overflow: ellipsis
isn't used. (I haven't tested with long ds project names)table maybe needs to be styled:
width: 100%;
table-layout: fixed;
or maybe something else?
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.
do we want to color rows differently when hovered? and, maybe more long term, but do we want to consider using InfiniteScrollTable? I'm not certain, but these feel like lower-priority things that we can change in later PRs or not do.
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.
iconNameBefore="add"
in<Button>
can be used instead ofIcon
.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.
could this be moved outside of the table body and moved right after the
if (isLoading){
block (i.e. before line ~27).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.
in the angular version, we had added the error message to the UI as we were trying to figure out (and still are) an issue with uwrapid account where DS project listings would fail intermittently. so it looked like this: https://github.com/TACC-Cloud/hazmapper/blob/main/angular/src/app/components/main-welcome/main-welcome.component.html#L44-L49
should we add the error message now or add later if needed?
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.
if there is no associated ds_project, we want to show ---------- like the angular code.
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.
could core components SectionMesage, Message or InlineMessage just be used instead of EmptyTablePlaceholder? both:
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.
Here we just want
maps
and notprojects
as in the frontend we have to avoid it unless referring to DS projects..in the angular version, it was:
do we want to keep it the same or tweak it a little?
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.
here are some comments for this component, but depending on #268 (comment), they might not be needed. so you might ignore this if EmptyPLaceholder is replaced by something else:
src/component/utils
? is that spot for smaller components that will be reused by things insrc/components
? if it is very Projects or ProjectListing and only used there, we could stick under src/components/Projects/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.
have a question on this but will put in slack.