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

ELEMENTS-1216: add route resolver to nuxeo-routing-behavior #271

Merged
merged 1 commit into from Nov 2, 2020

Conversation

Gabez0r
Copy link
Collaborator

@Gabez0r Gabez0r commented Aug 11, 2020

Required by nuxeo/nuxeo-web-ui#985.

This introduces a breaking change to people developing on top of the nuxeo-elements alone. People using on top of Web UI will not be affected. It can be fixed by either locking into a previous version or manually fixing their router.

@Gabez0r Gabez0r requested a review from a team as a code owner August 11, 2020 09:04
@nuxeojenkins
Copy link
Contributor

View issue in JIRA: ELEMENTS-1216: Allow nuxeo-document-suggestion to use both uid and path

Copy link
Contributor

@richardsd richardsd left a comment

Choose a reason for hiding this comment

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

If it is a breaking change, we need to make it clear in the release notes, no?

@nuxeo-web-ui-jx-bot
Copy link
Contributor

⭐ PR built and available in a preview environment nuxeo-nuxeo-elements-pr-271 here

richardsd
richardsd previously approved these changes Aug 18, 2020
@nuxeo-web-ui-jx-bot
Copy link
Contributor

⭐ PR built and available in a preview environment nuxeo-nuxeo-elements-pr-271 here

@nuxeo-web-ui-jx-bot
Copy link
Contributor

⭐ PR built and available in a preview environment nuxeo-nuxeo-elements-pr-271 here

@nuxeo-web-ui-jx-bot
Copy link
Contributor

⭐ PR built and available in a preview environment nuxeo-nuxeo-elements-pr-271 here

@Gabez0r Gabez0r marked this pull request as ready for review August 27, 2020 12:42
mnixo
mnixo previously approved these changes Aug 27, 2020
ui/nuxeo-routing-behavior.js Outdated Show resolved Hide resolved
richardsd
richardsd previously approved these changes Sep 1, 2020
@richardsd
Copy link
Contributor

The improvements seem clear.

@Gabez0r
Copy link
Collaborator Author

Gabez0r commented Oct 8, 2020

We are no longer introducing a breaking change here. We are now checking first if there is a route resolver defined, on nuxeo-document-suggestion, and if not, we keep the old behavior.

@Gabez0r Gabez0r changed the title ELEMENTS-1216: allow nuxeo-document-suggestion to use both uid and path ELEMENTS-1216: add route resolver to nuxeo-routing-behavior Oct 9, 2020
@nuxeo-web-ui-jx-bot
Copy link
Contributor

⭐ PR built and available in a preview environment nuxeo-nuxeo-elements-pr-271 here

ui/nuxeo-routing-behavior.js Show resolved Hide resolved
}
let entityType = obj['entity-type'];
if (!entityType) {
// XXX Sometimes we don't have the entity type. For example, on nuxeo-document-storage, we were not storing it.
Copy link
Contributor

Choose a reason for hiding this comment

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

can we fix this at nuxeo-document-storage level so we store the entity-type and add it in case it's missing on stored documents so we can do without this hack here ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could certainly fix this for newly added documents, but I'm not quite sure how to fix this for already existing documents in a reliable way, since we just forward the value of the inner iron-localstorage. We do have a get method which we use on some elements, but I don't think it would cover all use cases, specially those that rely on data binding.

Alternatively, we could try to do something with a new private property to hold the "fixe" set of documents, or we could try to run something when the value changes to fix the documents, but I'm not sure it is worth the effort?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can try to fix this on nuxeo-home, which is the only place by default where we generate URLs from docs stored on local storage, AFAIK. But I'm not sure I'd call this robust.

ui/nuxeo-routing-behavior.js Show resolved Hide resolved
ui/nuxeo-routing-behavior.js Show resolved Hide resolved
};

function setNuxeoRouterKey(entityType, value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

remark: we really need a window.Nuxeo.UI.config.set('...') or something...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

+1

ui/widgets/nuxeo-document-suggestion.js Outdated Show resolved Hide resolved
@nuxeo-web-ui-jx-bot
Copy link
Contributor

⭐ PR built and available in a preview environment nuxeo-nuxeo-elements-pr-271 here

// document` method and do not know how to handle paths
fn = (routeKey === 'path' && this.router.browse) || fn;
}
routeKey = routeKey || 'id'; // let `id` be the default key
Copy link
Contributor

Choose a reason for hiding this comment

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

🎵 let id be 🎵 let id be ...

nelsonsilva
nelsonsilva previously approved these changes Oct 27, 2020
mnixo
mnixo previously approved these changes Oct 29, 2020
ui/test/nuxeo-routing-behavior.test.js Outdated Show resolved Hide resolved
richardsd
richardsd previously approved these changes Oct 29, 2020
@nuxeo-web-ui-jx-bot
Copy link
Contributor

⭐ PR built and available in a preview environment nuxeo-nuxeo-elements-pr-271 here

@Gabez0r Gabez0r merged commit 33ec850 into master Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants