-
Notifications
You must be signed in to change notification settings - Fork 14
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
ENH Pre-render the LinkField in entwine #241
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
|
||
|
@@ -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?'); | ||
|
@@ -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; | ||
} | ||
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} | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
|
@@ -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 { | ||
|
@@ -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 | ||
|
@@ -155,10 +156,12 @@ const LinkPickerTitle = ({ | |
<span className="link-picker__title-text">{title}</span> | ||
{getVersionedBadge(versionState)} | ||
</div> | ||
<small className="link-picker__type"> | ||
{typeTitle}: | ||
<span className="link-picker__url">{description}</span> | ||
</small> | ||
{typeTitle && ( | ||
<small className="link-picker__type"> | ||
{typeTitle}: | ||
<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 | ||
|
@@ -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, | ||
|
@@ -198,4 +201,8 @@ LinkPickerTitle.propTypes = { | |
buttonRef: PropTypes.object.isRequired, | ||
}; | ||
|
||
LinkPickerTitle.defaultProps = { | ||
versionState: versionStates.unversioned, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
const versionStates = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,8 @@ | |
"php": "^8.1", | ||
"silverstripe/framework": "^5.2", | ||
"silverstripe/cms": "^5", | ||
"silverstripe/versioned": "^2" | ||
"silverstripe/versioned": "^2", | ||
"silverstripe/admin": "^2.2" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a dependency on admin 2.2 because of the new spinner template. |
||
}, | ||
"require-dev": { | ||
"silverstripe/frameworktest": "^1", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
namespace SilverStripe\LinkField\Form; | ||
|
||
use LogicException; | ||
use SilverStripe\ORM\ArrayList; | ||
use SilverStripe\ORM\DataObject; | ||
use SilverStripe\ORM\Relation; | ||
use SilverStripe\ORM\SS_List; | ||
|
@@ -132,4 +133,9 @@ private function loadFrom(DataObject $record): void | |
$value = array_values($relation->getIDList() ?? []); | ||
parent::setValue($value); | ||
} | ||
|
||
public function LinkIDs(): ArrayList | ||
{ | ||
return ArrayList::create($this->dataValue()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This wraps the IDs in an ArrayList so the SS template can loop over them. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,15 @@ | ||
<%-- This template is here to bootstrap a LinkField React form field --%> | ||
<%-- It includes some pre-rendered content to provide a nicer UI while waiting for React to boot --%> | ||
<%-- Once React is done pre-rendering, it will discard the pre-rendered markup --%> | ||
<input $AttributesHTML /> | ||
<div data-field-id="$ID" data-schema-component="$SchemaComponent" class="entwine-linkfield" data-types="$TypesProp"></div> | ||
<div data-field-id="$ID" data-schema-component="$SchemaComponent" class="entwine-linkfield" data-types="$TypesProp"> | ||
<div class="link-field__container"> | ||
<% include SilverStripe/LinkField/Form/LinkField_Spinner %> | ||
<div> | ||
<div class="link-picker__link link-picker__link--is-first link-picker__link--is-last form-control link-picker__link--disabled link-picker__link--published" role="button" aria-disabled="false" aria-roledescription="sortable" aria-describedby="" id="link-picker__link-42"> | ||
<button type="button" disabled="" class="link-picker__button font-icon-link btn btn-secondary disabled"></button> | ||
</div> | ||
</div> | ||
</div> | ||
</div> | ||
|
maxime-rainville marked this conversation as resolved.
Show resolved
Hide resolved
GuySartorelli marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<%-- This template is here to bootstrap a LinkField React form field --%> | ||
<%-- It includes a pre-rendered spinner to provide a nicer UI while waiting for React to boot --%> | ||
<%-- Once React is done pre-rendering, it will discard the pre-rendered markup --%> | ||
<div class="link-field__loading"> | ||
<div class="cms-content-loading-overlay ui-widget-overlay-light"></div> | ||
<% include SilverStripe/Admin/Includes/CMSLoadingSpinner %> | ||
</div> | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,35 @@ | ||
<%-- This template is here to bootstrap a MultiLinkField React form field --%> | ||
<%-- It includes some pre-rendered content to provide a nicer UI while waiting for React to boot --%> | ||
<%-- Once React is done pre-rendering, it will discard the pre-rendered markup --%> | ||
<input $AttributesHTML /> | ||
<div data-is-multi="true" data-field-id="$ID" data-schema-component="$SchemaComponent" class="entwine-linkfield" data-types="$TypesProp"></div> | ||
<div data-is-multi="true" data-field-id="$ID" data-schema-component="$SchemaComponent" class="entwine-linkfield" data-types="$TypesProp"> | ||
|
||
<div class="link-field__container"> | ||
<% include SilverStripe/LinkField/Form/LinkField_Spinner %> | ||
<div class="link-picker form-control"> | ||
<div class="link-picker__menu dropdown"> | ||
<button | ||
type="button" | ||
aria-haspopup="true" | ||
aria-expanded="false" | ||
class="link-picker__menu-toggle font-icon-plus-1 dropdown-toggle btn btn-secondary" | ||
aria-label="<%t SilverStripe\LinkField\Models\MultiLinkField.AddLink "Add link" %>"> | ||
<%t SilverStripe\LinkField\Models\MultiLinkField.AddLink "Add link" %> | ||
</button> | ||
</div> | ||
</div> | ||
<div class="link-picker-links"> | ||
<% loop $LinkIDs %> | ||
<div class="link-picker__link form-control link-picker__link--published <% if $IsFirst %>link-picker__link--is-first<% end_if %> <% if $IsLast %>link-picker__link--is-last<% end_if %>"> | ||
<button | ||
type="button" | ||
disabled="" | ||
class="link-picker__button font-icon-link btn btn-secondary disabled" | ||
aria-label="<%t SilverStripe\LinkField\Models\MultiLinkField.EditLink "Edit link" %>"></button> | ||
</div> | ||
<% end_loop %> | ||
</div> | ||
</div> | ||
|
||
</div> | ||
|
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.
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.