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.

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

Allow the Editor to fetch file details if the parent doesn't pass… #1225

Conversation

maxime-rainville
Copy link
Contributor

@maxime-rainville maxime-rainville commented Jul 16, 2021

Ready for review.

A couple edge cases to consider:

  • Editing a File and a Folder through the File section
  • Editing a File and a Folder through the WYSIWYG
  • Editing a File and a Folder through the UploadField

Parent issue

Copy link
Contributor Author

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

Could use some initial feedback before I get too far down this rabbit hole.

This thing mostly works, but needs a bit of cleaning

@@ -11,6 +12,7 @@ const registerQueries = () => {
Injector.query.registerFragment('FileInterfaceFields', fileInterface);
Injector.query.registerFragment('FileFields', file);
Injector.query.register('ReadFilesQuery', isLegacy ? readFilesQueryLegacy : readFilesQuery);
Injector.query.register('ReadOneFileQuery', isLegacy ? readFilesQueryLegacy : readOneFileQuery);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just have the GraphQL 4 version working for now ... need to do the GraphQL v3 equivalent as well.

if (props.file) {
return <Component {...props} />;
} else {
const newProps = {...props, fileId: props.targetId};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When file is NOT provided by the parent, we need to fire a GraphQL request to retrieve a single file as defined by props.targetId.

function magic(Component) {
return (props) => {
if (props.file) {
return <Component {...props} />;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When file is provided by the parent, we do nothing.

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 a scenario where AssetAdmin is not done loading the first page of file results, so it can't pass the file to the Editor.

When it's done, it provides the file, which causes the Editor to reload. I'm thinking either:

  • AssetAdmin communicates us its loading state and we just wait until it's done fetching its initial list before we start loading the editor or,
  • We bail on having AssetAdmin passing us the file info and always rely on our own GraphQL request all the time.

I'm leaning towards the second options. That would increase our number of request slightly, but it reduces the complexity of the component and the tie-in with AssetAdmin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had a chat with @emteknetnz. We decided to break the link between AssetAdmin and Editor and always fire a GraphQL request to check if targetID is a folder or a file.

import { hasFilters } from 'components/Search/Search';
import { graphqlTemplates } from 'lib/Injector';

const apolloConfig = {
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 file mostly copies readFilesQuery, but removes all the fluff.

We're still using the same graphQL endpoint to retrieve our single file. Maybe we should create an actual readOneFile query server side. Not sure if that's advisable.

client/src/state/files/readOneFileQuery.js Show resolved Hide resolved
client/src/state/files/readOneFileQuery.js Outdated Show resolved Hide resolved
client/src/state/files/readOneFileQuery.js Show resolved Hide resolved
…tor works even when viewing a file not on the current page
@maxime-rainville maxime-rainville force-pushed the pulls/1.8/editing-file-onsecond-page branch from 79e211a to b7061ce Compare July 19, 2021 05:31
@@ -323,12 +324,11 @@ class Editor extends Component {
Editor.propTypes = {
file: fileShape,
className: PropTypes.string,
targetId: PropTypes.number.isRequired,
fileId: PropTypes.number.isRequired,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've renamed this to fileId. This avoids us having to do gymnastic to change the prop from targetId to fileID when we pass it to the graphQL query and it just makes more sense since the target will always be a file ID any way.

};
},
props({ data: { error, readFiles, loading } }) {
const file = (readFiles && readFiles.nodes[0]) ? readFiles.nodes[0] : null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the only difference between v3.5 and 4. In 4, results are on the top level of readFiles

@maxime-rainville maxime-rainville changed the title WIP Allow the Editor to fetch file details if the parent doesn't pass… Allow the Editor to fetch file details if the parent doesn't pass… Jul 19, 2021
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Good

@emteknetnz emteknetnz merged commit 92b97a1 into silverstripe:1.8 Jul 19, 2021
@emteknetnz emteknetnz deleted the pulls/1.8/editing-file-onsecond-page branch July 19, 2021 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants