-
Notifications
You must be signed in to change notification settings - Fork 0
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
F t31116 demosplan UI no longer use routing #91
F t31116 demosplan UI no longer use routing #91
Conversation
- insert value in core.
- insert value in core.
… of string value.
… of string value.
…instead of string value.
…instead of string value.
@@ -53,7 +68,7 @@ export default { | |||
|
|||
computed: { | |||
userFilteredSegmentUrl () { | |||
return Routing.generate('dplan_segments_list', { procedureId: this.procedureId }) + '/' + this.userHash | |||
return Routing.generate(this.segmentsListRoute, { procedureId: this.procedureId }) + '/' + this.userHash |
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 we want to pass the complete route - the result of Routing.generate - not just the String that has to be generated.
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.
@@ -75,7 +90,7 @@ export default { | |||
} | |||
|
|||
// Get count of segments assigned to the current user | |||
const segmentUrl = Routing.generate('api_resource_list', { resourceType: 'StatementSegment' }) | |||
const segmentUrl = Routing.generate(this.resourceListRoute, { resourceType: 'StatementSegment' }) |
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.
see above and all other changes
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.
- change props names.
- we need to rebuild this function.
- we should move the component to demosplan-core and give it a more specific name, too.
- make this prop optional and add condition.
Done in 57ffba6 |
Co-authored-by: hwiem <40487668+hwiem@users.noreply.github.com>
- because we move this component to core.
- edit in package.json to test the changes in core.
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.
Please Check sonarCloud
CHANGELOG.md
Outdated
### Changed | ||
- ([#91](https://github.com/demos-europe/demosplan-ui/pull/91)) Pass routes as props ([@ahmad-demos](https://github.com/ahmad-demos)) |
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.
this is BREAKING.
And we should mention That Routing.generate
is not used internally anymore. (in most the places)
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 I understood correctly, breaking changes are not mentioned as such until version 1?
Also, I'm not sure we need to mention internal changes?
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. I thought we should mention this, because its not internal since You have to pass (other) props/values in the components.
Its just possible to make braeaking changes without bumping a major version (but with telling in the chagnelog). That was, what was I thought.
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.
but I am not sure at all
# Conflicts: # CHANGELOG.md
…use_routing' into f_T31116_demosplan_ui_no_longer_use_routing
https://yaits.demos-deutschland.de/T31116