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

Render trait display names everywhere #472

Merged

Conversation

vlagrassa
Copy link
Collaborator

Updates all pages/widgets that deal with traits to use the full trait display names. This includes:

  • Phenotype database table rows
  • Phenotype database offcanvas header
  • My Report / All Reports table rows
  • Trait selectors (single and double) (when creating/submitting a report)
  • Reports page sidebar
  • Reports page graph axis labels

Notes:

  • The PhenotypeReport objects cache the display name, so we can render them in the My Reports page without having to ping the database 1~2x for every report. I've added a DbOp that re-computes these cached names, just in case
  • For now, the Zhang traits just use the full CaeNDR name as the display_name_1 field. If we wanted to, we could try to split these names up based on the columns in the sheet.
Screenshot 2024-02-29 at 5 52 26 PM

Fixes rendering error if tags list is shorter than other columns (height-wise)
Replaces ad-hoc object construction with a structured query.  This is the same query we run to fill out the offcanvas sidebar.

Removes `data-description` and `data-species` fields from table rows, since we're now looking this information up in the database directly.
Store as Jinja dict linked to tool bp name. Rendered with macros.
Adds new class method for `PhenotypeReport` to recompute all cached names.
- Replace `trait_{n}_name` with `trait_{n}_name_caendr`
- Compute display name(s) when creating new report & in the recompute function
- Now use `trait_{n}_name_display` directly to render report page
Copy link
Collaborator

@r-vieira r-vieira left a comment

Choose a reason for hiding this comment

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

looks good. See inline comments.

src/modules/db_operations/seed_trait_files.py Show resolved Hide resolved
src/modules/site-v2/templates/_scripts/trait_utils.js Outdated Show resolved Hide resolved
});
}
}
// function filter_by_species(species) {
Copy link
Collaborator

@r-vieira r-vieira Apr 8, 2024

Choose a reason for hiding this comment

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

we can remove commented code

Copy link
Collaborator

@r-vieira r-vieira left a comment

Choose a reason for hiding this comment

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

looks good.

src/modules/db_operations/seed_trait_files.py Show resolved Hide resolved
@r-vieira r-vieira merged commit eb23b3f into development Apr 18, 2024
Copy link
Collaborator

@r-vieira r-vieira left a comment

Choose a reason for hiding this comment

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

looks good. See inline comments

.fail((error) => {
// If an error message was received, display it, otherwise use a fallback message
console.error(error);
const resp = error.responseJSON
Copy link
Collaborator

Choose a reason for hiding this comment

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

if throwing from line 144, will the property .responseJSON be present in the error object?

Also, it might be helpful to document the expected shape of the error.responseJSON structure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, this would be an API call error, not a JS error from the success block.

As far as I can tell, error objects always have a responseJSON property, although I do check in the next line to make sure the object exists (if (resp ...)

// If an error message was received, display it, otherwise use a fallback message
console.error(error);
const resp = error.responseJSON
if (resp && resp.message && false) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we clarify the intention here? assuming resp is object/null, resp.message is undefined/string/object then using false appears to convolute the intention with mixed types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm... I think I threw false in here because I wanted to keep this structure (which I use elsewhere for error handling) but I only want to flash the default message? Usually it's if (resp && resp.message), to just check if the message field exists. Definitely weird though, I'll clean it up.

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.

None yet

3 participants