Made digital specimen cards more specific and according to their UI#258
Made digital specimen cards more specific and according to their UI#258MelanieTheLion wants to merge 6 commits into
Conversation
|
❌ The last analysis has failed. |
| hidden: boolean; | ||
| } | ||
| }, | ||
| extraItem?: { |
There was a problem hiding this comment.
Is this the way to go here?
|
|
|
||
| a { | ||
| color: inherit; | ||
| text-decoration: underline; |
There was a problem hiding this comment.
shouldn't this actually be the default? It is probably removed because a is also styled as a button?
There was a problem hiding this comment.
No, color: inherit means the color is black, instead of the normal link color --indigo-11, and our links have text-decoration: none, but this citation has text-decoration: underline. It breaks with our normal link styling, because it is styled as a citation box now :)
| type FragmentType = 'verbatim' | 'copy' | 'default' | 'url'; | ||
|
|
||
| interface Props { | ||
| fragment: { |
There was a problem hiding this comment.
if isHtml is always false why even include it?. This is the case for multiple interfaces across files
There was a problem hiding this comment.
In IdentificationCard and GeoreferenceCard isHtml can be true (or false) or should be true, so I have included it.
| const { copy } = useClipboard(); | ||
|
|
||
| if (!item.value || item.hidden) return null; | ||
| if (item.value === undefined || item.value === null || item.value === '' || item.hidden) { |
There was a problem hiding this comment.
item.value === undefined || item.value === null || item.value === ''
This seems like a problem that should be solved when fetching this kind of data. As in, that you should transform the data before using it in the render components. There shouldn't be 3 ways to indicate that a value is empty. There will probably be more times in the future that you need to check if it is empty, then there is a change that you will forget 1 or more of the empty options
| import './Cards.scss'; | ||
|
|
||
| type FragmentType = 'verbatim' | 'copy' | 'default' | 'url'; | ||
| type isHtmlFragmentType = 'verbatim' | 'copy' | 'default'; |
There was a problem hiding this comment.
You can use Type Utilities and/or extending and merging types for creating new types from others.
type isHtmlFragmentType = Exclude<FragmentType, 'url'>;|
|
||
| interface Props { | ||
| fragment: { | ||
| scientificName: { label: string, value: string, isHtml: true, type: isHtmlFragmentType, hidden: boolean }, |
There was a problem hiding this comment.
There's a lot of repetition going on with these types (in multiple files). There's the rule of three: When you're repeating the same (kind of) code 3 times, it's time for an abstraction.
There was a problem hiding this comment.
Maybe that's something for a larger refactor later?
| </Button> | ||
| </div> | ||
| <div className="digital-specimen-card-content"> | ||
| <LabelValuePair item={fragment['scientificName']} ></LabelValuePair> |
There was a problem hiding this comment.
This looks like a description list (<dl>) to me. Also when taking the design into consideration.
There was a problem hiding this comment.
And here we have the term <dt> and description <dd> elements for the parent <dl>.
You are allowed to wrap them in a <div> to make rows:
https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/dl#wrapping_name-value_groups_in_div_elements
3 participants
In this PR:
The DigitalSpecimenCard was first a reusable component for all 5 dataCards. But I ran into issues that made the reusable component become way too complicated. So I decided to separate concerns and create separate components for each data card. More code and more components, but easier to maintain in the long run.
UI still needs tweaks, but I have outstanding questions I have to solve first.
This page is not officially live yet. Because it is so big, we are working in iterations.
UX Design
Not in this pr:
What it looks like now:
