Annotations: FE#6481
Conversation
|
…attern - Show the site option as a disabled radio with a plan pill and hover tooltip instead of an inline warning message - Add a reusable UpgradePill with diamond icon in both React and LiveView and use it in the annotation, segment, and goal settings modals plus the feature setup notice
| </div> | ||
| ) | ||
|
|
||
| export const AnnotationAttributionLine = ({ |
There was a problem hiding this comment.
Minor nit: the word "attribution" threw me off a bit when first reading it. How about "signature" instead?
| return formatDay(date) | ||
| } | ||
|
|
||
| const NOTE_RECOMMENDED_MAX_LENGTH = 250 |
There was a problem hiding this comment.
Why the word "recommended"? Same question about the UI label
| const NOTE_RECOMMENDED_MAX_LENGTH = 250 | |
| const NOTE_MAX_LENGTH = 250 |
| const defaultNote = '' | ||
| const [note, setNote] = useState(defaultNote) |
There was a problem hiding this comment.
This makes it look like the default value is needed elsewhere. Let's inline?
| const defaultNote = '' | |
| const [note, setNote] = useState(defaultNote) | |
| const [note, setNote] = useState('') |
| const saveableNote = trimmedNote.length | ||
| ? trimmedNote | ||
| : notePlaceholder |
There was a problem hiding this comment.
Shouldn't we prohibit creating an empty note rather than saving the note with notePlaceholder? When you open the "Add note" modal and hit save directly, it creates this note:
| notePlaceholder: string | ||
| } | ||
|
|
||
| export const CreateAnnotationModal = ({ |
There was a problem hiding this comment.
I think notePlaceHolder and initialType shouldn't be props, but rather fixed values defined as module level consts. They do make sense as props in the "Edit note" modal, but not in the "Add note" one.
| }: AnnotationModalProps & | ||
| ApiRequestProps & { | ||
| onSave: (input: Pick<Annotation, 'id' | 'note' | 'type'>) => void | ||
| onDelete?: (annotation: Annotation) => void |
There was a problem hiding this comment.
Looks like this function is always provided, so we could enforce this prop and remove the typeof onDelete === 'function' check below?
| ) | ||
|
|
||
| it.each([[AnnotationType.personal], [AnnotationType.site]])( | ||
| 'forbids the public role from adding %s annotations', |
There was a problem hiding this comment.
Should we test the same for super admins?
| Role.owner | ||
| ] | ||
|
|
||
| export function canEditAnnotation({ |
There was a problem hiding this comment.
The whole function body is very similar to canAddAnnotation. Maybe extract a private function like hasPermissions({type, user, siteAnnotationsAvailable}) to not repeat this logic?
| if (tooltip.persistent && isTouchDevice === false) { | ||
| document.addEventListener('click', onClickOutside) | ||
| } else { | ||
| document.removeEventListener('click', onClickOutside) | ||
| } |
There was a problem hiding this comment.
The else branch should be unnecessary because the return function always cleans the eventListener up.
| {!!annotationDatetime && canAddAnnotation && ( | ||
| <Button | ||
| size="xs" | ||
| className="flex-1 bg-gray-600/70 border-gray-600/70 hover:bg-gray-600 hover:border-gray-600" |
There was a problem hiding this comment.
The classname between the two buttons should be exactly the same so perhaps let's extract that into a variable?
3 participants
Changes
Adds the rest of Annotations feature. Depends on the BE PR.
Tests
Changelog
Documentation
Dark mode
TODO after merge