-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Feat: Enable hosting app in any route, with easy config #3722
Conversation
@peterlau, any updates here? Please let me know if there's something I can do further in this PR |
ui/src/utils/helpers.js
Outdated
@@ -1,5 +1,7 @@ | |||
import { format, formatDuration, intervalToDuration } from "date-fns"; | |||
import _ from "lodash"; | |||
import { isEmpty as _isEmpty } from 'lodash'; |
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 recommend using import _isEmpty from 'lodash/isEmpty'
.
Btw the above line imported lodash
, so you need to use _.isEmpty
only, don't need to re-import.
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.
Thanks for the feedback. I have fixed this by removing my duplicate import and using _.isEmpty
ui/src/utils/helpers.js
Outdated
basename = new URL(packageJson.homepage).pathname; | ||
} catch(e) {} | ||
return _isEmpty(basename) ? '/' : basename; | ||
} |
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.
Missing new line at end of file.
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.
Added a new line with the latest commit
Fixed review comments and rebased my commits with upstream. |
ui/src/plugins/fetch.js
Outdated
|
||
export function cleanDuplicateSlash(path) { | ||
return path.replace(/([^:]\/)\/+/g, "$1"); | ||
} |
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 file is missing new line at the end of file too.
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.
Done
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.
looks good to me.
@nhandt2021, @peterlau - could we get this PR merged if there are no further changes required? |
I don't have permission to do that 😄 |
This PR is stale, because it has been open for 45 days with no activity. Remove the stale label or comment, or this will be closed in 7 days. |
@peterlau - could we get this PR merged if there are no further changes required? |
Thanks! |
Pull Request type
./gradlew generateLock saveLock
to refresh dependencies)Changes in this PR
Describe the new behavior from this PR, and why it's needed
Issue #3656
Alternatives considered
All details are in the issue attached with this PR