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

task/WG-232-React-Listing-UI #268

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

sophia-massie
Copy link
Contributor

@sophia-massie sophia-massie commented Oct 3, 2024

Overview:

PR Status:

  • Ready.
  • Work in Progress.
  • Hold.

Related Jira tickets:

Summary of Changes:

Uses styles from Hazmapper angular to create Project Listing and cleans up the main project listing page

Testing Steps:

  1. Run nvm use 20 and npm ci
  2. Run react front end and go to http://localhost:4200/. Check the UI so that it matches the Jira expectations

UI Photos:

Screenshot 2024-10-03 at 3 30 43 PM

Error message:
Screenshot 2024-10-03 at 6 15 46 PM

No projects message:
Screenshot 2024-10-03 at 6 16 25 PM

Notes:

- Brings over EmptyTablePlaceholder from TUP UI
that CMD collaborated with us on
type 'NonNullable<ReactNodeLike>' error in unit test
@sophia-massie sophia-massie changed the title task/WG-232-React-Listing-UI-clean task/WG-232-React-Listing-UI Oct 4, 2024
Copy link
Collaborator

@nathanfranklin nathanfranklin left a comment

Choose a reason for hiding this comment

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

Awesome 👍 left some comments.

react/package.json Show resolved Hide resolved
Comment on lines +49 to +64
{isError && (
<tr>
<td colSpan={3}>
<EmptyTablePlaceholder type="error">
There was an error gathering your maps and projects. {''}
<a
href="https://www.designsafe-ci.org/help/new-ticket/"
target="_blank"
rel="noreferrer"
>
Click here to submit a ticket on DesignSafe.
</a>
</EmptyTablePlaceholder>
</td>
</tr>
)}
Copy link
Collaborator

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).

 if (isError) {
    return (
      <EmptyTablePlaceholder type="error">
        There was an error gathering your maps and projects. {''}
        <a
          href="https://www.designsafe-ci.org/help/new-ticket/"
          target="_blank"
          rel="noreferrer"
        >
          Click here to submit a ticket on DesignSafe.
        </a>
      </EmptyTablePlaceholder>
    );
  }

Copy link
Collaborator

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?

) : (
<tr>
<td colSpan={3}>
<EmptyTablePlaceholder type="info">
Copy link
Collaborator

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:

  • here where there are no maps
  • and also when there is an error

<tr>
<td colSpan={3}>
<EmptyTablePlaceholder type="info">
No projects or maps found.
Copy link
Collaborator

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 not projects as in the frontend we have to avoid it unless referring to DS projects..

in the angular version, it was:

                            <div id="welcome-no-information">
                                <div>No Map Exists!</div>
                                <a (click)="openCreateProjectModal()"> Create a New Map. </a>
                            </div>

do we want to keep it the same or tweak it a little?

@@ -8,3 +10,18 @@
}

*/

/* CSS module styles taken from Angular styles.styl */
Copy link
Collaborator

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.

<Icon name="edit-document"></Icon>
<div className={styles.root}>
<div className={styles.projectList}>
<table>
Copy link
Collaborator

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?

type="link"
className={styles.projectListItemButton}
>
<Icon name="add" />
Copy link
Collaborator

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 of Icon.

Comment on lines +70 to +72
{proj.ds_project?.value.projectId}
{' - '}
{proj.ds_project?.value.title}
Copy link
Collaborator

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.

<div className={styles.projectList}>
<table>
<thead className={styles.projectHeader}>
<tr>
Copy link
Collaborator

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.

@@ -0,0 +1,25 @@
import { SectionMessage } from '../../core-components';
Copy link
Collaborator

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:

  • do we want put this in src/component/utils? is that spot for smaller components that will be reused by things in src/components? if it is very Projects or ProjectListing and only used there, we could stick under src/components/Projects/
  • the no children handling of "No data available" is never used. drop?

nathanfranklin pushed a commit that referenced this pull request Oct 23, 2024
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