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.

Sign up for GitHub

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

Jump to bottom

ENH Pre-render the LinkField in entwine #241

Merged
merged 5 commits into from
Mar 12, 2024
Merged

ENH Pre-render the LinkField in entwine #241

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8a891cd
ENH Pre-render the LinkField in entwine
maxime-rainville Mar 1, 2024
281a934
MNT Correct disabled link Jest test
maxime-rainville Mar 7, 2024
8d0b81f
ENH Move versioned states to a constant
maxime-rainville Mar 9, 2024
684dcc5
ENH Use global Spinner template
maxime-rainville Mar 9, 2024
26d7050
MNT Revert test change for aria-disabled
maxime-rainville Mar 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6,780 changes: 6,779 additions & 1 deletion client/dist/js/bundle.js
View file
Open in desktop

Large diffs are not rendered by default.

221 changes: 220 additions & 1 deletion client/dist/styles/bundle.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 13 additions & 6 deletions client/src/components/LinkField/LinkField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import i18n from 'i18n';
import url from 'url';
import qs from 'qs';
import classnames from 'classnames';
import versionStates from 'constants/versionStates';

export const LinkFieldContext = createContext(null);

Expand Down Expand Up @@ -267,7 +268,11 @@ const LinkField = ({
*/
const handleDelete = (linkID, deleteType) => {
const versionState = data[linkID]?.versionState || '';
const isVersioned = ['draft', 'modified', 'published'].includes(versionState);
const isVersioned = [
versionStates.draft,
versionStates.modified,
versionStates.published
].includes(versionState);
const deleteText = isVersioned
? i18n._t('LinkField.ARCHIVE_CONFIRM', 'Are you sure you want to archive this link?')
: i18n._t('LinkField.DELETE_CONFIRM', 'Are you sure you want to delete this link?');
Expand Down Expand Up @@ -348,12 +353,14 @@ const LinkField = ({
const links = [];
for (let i = 0; i < linkIDs.length; i++) {
const linkID = linkIDs[i];
// Only render items we have data for
const linkData = data[linkID];
if (!linkData) {
// Render dataless item to provide a good loading experience, except if we have single link field
const linkData = data[linkID] || {};
if (!linkData && !isMulti) {
continue;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By not skipping the link entries for which we don't have data yet, we'll render a list of placeholder LinkPickerTitle. This will avoids having a big jump on the page once the data finally gets back to us.

}
const type = types.hasOwnProperty(linkData.typeKey) ? types[linkData.typeKey] : {};
const type = types.hasOwnProperty(linkData.typeKey) ?
types[linkData.typeKey] :
{icon: 'font-icon-link' };
links.push(<LinkPickerTitle
key={linkID}
id={linkID}
Expand Down Expand Up @@ -446,7 +453,7 @@ const LinkField = ({

const saveRecordFirst = !loadingError && ownerID === 0;
const renderLoadingError = loadingError;
const renderPicker = !loadingError && !inHistoryViewer && !saveRecordFirst && (isMulti || Object.keys(data).length === 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was needed because a disabled LinkField with pe-existing data would briefly render a picker and a place holder link.

With this change, it only renders the picker.

const renderPicker = !loadingError && !inHistoryViewer && !saveRecordFirst && (isMulti || linkIDs.length === 0);
const renderModal = !loadingError && !saveRecordFirst && Boolean(editingID);
const loadingErrorText = i18n._t('LinkField.FAILED_TO_LOAD_LINKS', 'Failed to load link(s)');
const saveRecordFirstText = i18n._t('LinkField.SAVE_RECORD_FIRST', 'Cannot add links until the record has been saved');
Expand Down
22 changes: 20 additions & 2 deletions client/src/components/LinkField/tests/LinkField-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,30 @@ test('LinkField returns list of links if they exist', async () => {
});

test('LinkField will render disabled state if disabled is true', async () => {
const { container } = render(<LinkField {...makeProps({
ownerID: 1,
disabled: true
})}
/>);
await doResolve({ json: () => ({
123: {
title: 'Page title',
typeKey: 'mylink',
}
}) });
await screen.findByText('Page title');
const linkPicker = container.querySelectorAll('#link-picker__link-123');
expect(linkPicker[0]).toHaveAttribute('aria-disabled', 'false');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an aria-disabled attribute on the Disabled linkfield ... but its value is false. I'm not sure if that's actually what we want to do here.

I've added like this for now. We can revisit in the accessibility card.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(linkPicker[0]).toHaveAttribute('aria-disabled', 'false');

Adding it here make it look like this is the correct behaviour, even though we know it's wrong. If we're explicitly adding aria-disabled on the other card then we'll naturally add a unit test for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the test for false.

expect(linkPicker[0]).toHaveClass('link-picker__link--disabled');
});

test('Empty disabled LinkField will display cannot edit message', async () => {
const { container } = render(<LinkField {...makeProps({
ownerID: 1,
disabled: true,
value: undefined
emteknetnz marked this conversation as resolved.
Show resolved Hide resolved
})}
/>);
doResolve();
await screen.findByText('Cannot create link');
expect(container.querySelectorAll('.link-picker')).toHaveLength(1);
expect(container.querySelectorAll('.link-picker')[0]).toHaveTextContent('Cannot create link');
Expand Down Expand Up @@ -176,7 +194,7 @@ test('LinkField will render loading indicator if ownerID is not 0', async () =>
/>);
expect(container.querySelectorAll('.link-field__save-record-first')).toHaveLength(0);
expect(container.querySelectorAll('.link-field__loading')).toHaveLength(1);
expect(container.querySelectorAll('.link-picker')).toHaveLength(1);
expect(container.querySelectorAll('.link-picker')).toHaveLength(0);
});

test('LinkField will render link-picker if ownerID is not 0 and isMulti and has finished loading', async () => {
Expand Down
25 changes: 16 additions & 9 deletions client/src/components/LinkPicker/LinkPickerTitle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { LinkFieldContext } from 'components/LinkField/LinkField';
import { Button } from 'reactstrap';
import { useSortable } from '@dnd-kit/sortable';
import { CSS } from '@dnd-kit/utilities';
import versionStates from 'constants/versionStates';

const stopPropagation = (fn) => (e) => {
e.nativeEvent.stopImmediatePropagation();
Expand All @@ -19,10 +20,10 @@ const stopPropagation = (fn) => (e) => {
const getVersionedBadge = (versionState) => {
let title = '';
let label = ''
if (versionState === 'draft') {
if (versionState === versionStates.draft) {
title = i18n._t('LinkField.LINK_DRAFT_TITLE', 'Link has draft changes');
label = i18n._t('LinkField.LINK_DRAFT_LABEL', 'Draft');
} else if (versionState === 'modified') {
} else if (versionState === versionStates.modified) {
title = i18n._t('LinkField.LINK_MODIFIED_TITLE', 'Link has unpublished changes');
label = i18n._t('LinkField.LINK_MODIFIED_LABEL', 'Modified');
} else {
Expand Down Expand Up @@ -108,11 +109,11 @@ const LinkPickerTitle = ({
classes[`link-picker__link--${versionState}`] = true;
}
const className = classnames(classes);
const deleteText = ['unversioned', 'unsaved'].includes(versionState)
const deleteText = [versionStates.unversioned, versionStates.unsaved].includes(versionState)
? i18n._t('LinkField.DELETE', 'Delete')
: i18n._t('LinkField.ARCHIVE', 'Archive');
const ariaLabel = i18n._t('LinkField.EDIT_LINK', 'Edit link');
if (['draft', 'modified'].includes(versionState)) {
if ([versionStates.draft, versionStates.modified].includes(versionState)) {
onUnpublishedVersionedState();
}
// Remove the default tabindex="0" attribute from the sortable element because we're going to manually
Expand Down Expand Up @@ -155,10 +156,12 @@ const LinkPickerTitle = ({
<span className="link-picker__title-text">{title}</span>
{getVersionedBadge(versionState)}
</div>
<small className="link-picker__type">
{typeTitle}:&nbsp;
<span className="link-picker__url">{description}</span>
</small>
{typeTitle && (
<small className="link-picker__type">
{typeTitle}:&nbsp;
<span className="link-picker__url">{description}</span>
</small>
)}
</div>
{(canDelete && !readonly && !disabled) &&
// This is a <span> rather than a <Button> because we're inside a <Button> and
Expand All @@ -180,7 +183,7 @@ LinkPickerTitle.propTypes = {
id: PropTypes.number.isRequired,
title: PropTypes.string,
description: PropTypes.string,
versionState: PropTypes.string,
versionState: PropTypes.oneOf(Object.values(versionStates)),
typeTitle: PropTypes.string.isRequired,
typeIcon: PropTypes.string.isRequired,
onDelete: PropTypes.func.isRequired,
Expand All @@ -198,4 +201,8 @@ LinkPickerTitle.propTypes = {
buttonRef: PropTypes.object.isRequired,
};

LinkPickerTitle.defaultProps = {
versionState: versionStates.unversioned,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change addresses the problem where the place holder LinkPickerTitle would get a draft state icon.

}

export default LinkPickerTitle;
9 changes: 9 additions & 0 deletions client/src/constants/versionStates.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const versionStates = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Repeating the same strings over and over again seemed wrong and likely to lead to confusion. So I moved them to a constant.

Copy link
Member

Choose a reason for hiding this comment

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

Could you please refrain from doing random refactoring half-way through a peer review. It adds unnecessary burden at the end of the person doing the peer review.

draft: 'draft',
modified: 'modified',
unversioned: 'unversioned',
unsaved: 'unsaved',
published: 'published',
};

export default versionStates;
Loading
Loading