-
Notifications
You must be signed in to change notification settings - Fork 5
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
EES-2701 glossary plugin #4218
EES-2701 glossary plugin #4218
Conversation
onSubmit: (item: GlossaryItem) => void; | ||
} | ||
|
||
export default function InsertGlossaryItemForm({ onCancel, onSubmit }: Props) { |
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.
Would name this GlossaryItemInsertForm
to follow normal component naming conventions
export default function InsertGlossaryItemForm({ onCancel, onSubmit }: Props) { | ||
const config = useConfig(); | ||
|
||
const listGlossaryQuery = useMemo(() => glossaryQueries.list(), []); |
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.
Don't need to memoize this, can just inline it i.e.
const { data: glossary = [], isLoading } = useQuery({
...glossaryQueries.list(),
staleTime: Infinity,
});
const glossaryQueries = createQueryKeys('glossary', { | ||
list() { | ||
return { | ||
queryKey: ['glossary'], |
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.
Don't need this query key. createQueryKeys
will automatically generate the query key as something like ['glossary', 'list']
.
Doing the above would end up with a key like ['glossary', 'list', 'glossary']
, which is unnecessary.
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.
I'm not quite sure what you're recommending here - queryKey
is required, I can't just have:
const glossaryQueries = createQueryKeys('glossary', {
list() {
return {
queryFn: () => glossaryService.listEntries(),
};
},
});
Do you mean I should just have an empty string as the queryKey (queryKey: ['']
) or that I don't need to use 'createQueryKeys' at all?
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.
Realised that it's not actually super clear how you define argument-less query keys.
Think this should potentially work:
const glossaryQueries = createQueryKeys('glossary', {
list: {
queryKey: null,
queryFn: () => glossaryService.listEntries(),
},
});
Note that it's no longer a callable query key anymore as it doesn't accept any args, so we'd just use it like:
useQuery(glossaryQueries.list);
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.
Yep, that works. Cheers.
onReady={handleReady} | ||
/> | ||
<Modal | ||
title="Insert glossary link" |
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.
<FormFieldTextInput | ||
formGroupClass="govuk-!-margin-top-5 govuk-!-margin-bottom-2" | ||
id="text" | ||
label="Edit link text" |
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.
Just 'Link text' would be sufficient as a label
name="text" | ||
/> | ||
|
||
<p>Slug: {form.values.slug}</p> |
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.
If we make the UI changes suggested, this would be better directly beneath the glossary entry field.
staleTime: Infinity, | ||
}); | ||
|
||
const allGlossaryEntries = glossary.flatMap(g => g.entries); |
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.
Could memoize this
import { useQuery } from '@tanstack/react-query'; | ||
import glossaryQueries from 'src/queries/glossaryQueries'; |
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.
Unneeded imports?
@@ -1,7 +1,7 @@ | |||
import { useCommentsContext } from '@admin/contexts/CommentsContext'; | |||
import styles from '@admin/components/editable/EditableContentForm.module.scss'; | |||
import FormFieldEditor from '@admin/components/form/FormFieldEditor'; | |||
import { Element } from '@admin/types/ckeditor'; | |||
import { Element, GlossaryItem } from '@admin/types/ckeditor'; |
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.
Unneeded import?
<InsertGlossaryItemForm | ||
onCancel={toggleGlossaryModal.off} | ||
onSubmit={item => { | ||
glossaryPlugin.current?.addGlossaryItem(item); |
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.
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.
Clicking on the read only blocks is meant to open the editor, but currently when you click on a link it'll open that link instead, which probably isn't what you'd want as it will take you away from editing the release. Adding GlossaryEntryButton
means that it'll flash up the modal briefly then open the editor.
So I've done this:
- prevented links from opening in read only blocks, so if you click on a link it'll open the editor like when you click anywhere else in the block.
- added
GlossaryEntryButton
so the icon shows but disabled the modal from opening, so when you click on them it'll open the editor.
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.
I think there's some accessibility problems with this approach.
Screen reader users will still be able to tab to GlossaryEntryButton
and the screen reader will announce that it's a button that you can show a glossary term definition for. This could be quite confusing when it actually does something completely different by changing the block to editing mode.
A similar thing will happen with links in that the screen reader will announce them, but the behaviour will not be what they're expecting.
There's two options from what I can see:
- Leave interactive elements (buttons, links, etc) to work normally and don't toggle to editing mode if they directly interacted with one of these
- Make any interactive elements within the preview container non-interactive
Option 2 seems preferable if we want to streamline the editing experience (as users don't need to tab through all interactive buttons).
There's a whole bunch of techniques for implementing option 2, but the simplest seems to be to add the inert
attribute to a parent element. Seems like support is pretty good across browsers nowadays.
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.
Yeah good point. I've gone for option 2 using inert
(with a workaround because of facebook/react#17157)
6188b4a
to
e7ffbda
Compare
2239fc7
to
dc91dd0
Compare
{/* Workaround to use inert, see https://github.com/facebook/react/issues/17157 */} | ||
{/* eslint-disable-next-line react/jsx-props-no-spreading */} | ||
<div {...{ inert: '' }}> |
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.
It'd be better to do this via ambient declarations so we can have access to inert
globally:
// types/react.d.ts
declare module 'react' {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
interface HTMLAttributes<T> {
inert?: '' | undefined;
}
}
declare global {
namespace JSX {
interface IntrinsicAttributes {
inert?: '' | undefined;
}
}
}
export {};
Also remember to re-export in types.index.d.ts
:
export * from './govuk-frontend';
export * from './memoizee';
+ export * from './react';
export * from './react-app-env';
export * from './recharts';
export * from './util';
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.
ok, that's done now.
Adds a CKEditor plugin to insert glossary entries from the available list.
The plugin itself is very simple, it calls a function in the admin app which displays a modal to select a glossary entry and then inserts it as a link. Having the modal in the app instead of within CKEditor makes it much simpler to have a searchable list and means we can make changes to it more easily.
Once added the glossasry links behave like normal links in CKEditor. As currently, glossary links in the release preview and published releases have icons and when clicked show the entry in a modal.
Also:
Toolbar button:
Modal: