-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Added inline if-else to remove click function and hover pointer class #1555
Conversation
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.
nice getting this in a PR so fast! :)
className={`${classes.td} ${classes.theadLabel}`} | ||
onClick={() => handleSort(header.id)} | ||
className={ | ||
i == 8 |
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.
what does 8 mean?
this feels fragile, the design folks keep changing the required columns. consider structuring it to make it clearer why this code is 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.
tdm-calculator/client/src/components/Projects/ProjectsPage.js
Lines 366 to 385 in 8c0d6bc
const headerData = [ | |
{ | |
id: "dateHidden", | |
label: "Visibility" | |
}, | |
{ | |
id: "dateSnapshotted", | |
label: "Status" | |
}, | |
{ id: "name", label: "Name" }, | |
{ id: "address", label: "Address" }, | |
{ id: "VERSION_NO", label: "Alternative Number" }, | |
{ id: "firstName", label: "Created By" }, | |
{ id: "dateCreated", label: "Created On" }, | |
{ id: "dateModified", label: "Last Modified" }, | |
{ | |
id: "contextMenu", | |
label: "" | |
} | |
]; |
The function iterates through this list of headers and adds classes and click functions. I'm just using the index on this list of headers. I agree that it's fragile! Another option would be to add conditional based on label
being blank string, or maybe we can add a boolean property to this header list.... something like - "sortfunction": true/false
and base the conditionals off of that. I prefer the latter - I think it's more explicit. What do you think?
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.
Instead of
<td
key={i}
className={
i == 8
...
, it might be safer to write
<td
key={h.id}
className={
h.id === "contextMenu"
...
and you wouldn't need to use the index i
at all, since the purpose of the id property is to uniquely identify each column heading. This would be fairly robust with respect to future changes that might re-arrange colums.
…into 1552-remove-filtering-on-context-menu
@entrotech @ZekeAranyLucas Here are my changes - 38a527c |
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.
much clearer! ship it
Fixes ##1552
What changes did you make?
Why did you make the changes (we will use this info to test)?
Issue-Specific User Account
If you registered a new, temporary TDM User Account for this issue, indicate the
username (i.e., email address) for the account.
Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
Visuals before changes are applied
Visuals after changes are applied