feat(markdown-nav): add children url property #1727
Conversation
christianmemije
force-pushed
the
feat/markdown-nav-children-url
branch
from
5e022bb
to
bba9ac9
Compare
| { | ||
| title: '🔥', | ||
| childrenUrl: 'https://api.myjson.com/bins/aqnfk', | ||
| }, |
There was a problem hiding this comment.
Demo doesn't really appear to be working. Can you put in the url to here maybe:
https://raw.githubusercontent.com/Teradata/covalent/develop/README.md
There was a problem hiding this comment.
Fixed. childrenUrl has to point to a json not a .md.
There was a problem hiding this comment.
We should probably add a validation for this and an error state also.
| childrenUrl: 'https://api.myjson.com/bins/aqnfk', | ||
| }, | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Can you add a demo for the childUrl with also the jump to?
There was a problem hiding this comment.
I think there should be a separate demo just demoing startAt, but that can be done a separate PR?
| return await this._http | ||
| .get<IMarkdownNavigatorItem[]>(sanitizedUrl, { ...item.httpOptions }) | ||
| .toPromise(); | ||
| } catch { |
There was a problem hiding this comment.
We probably need an error state in case it fails.
There was a problem hiding this comment.
Also.. not sure if we should allow them to be able to go back while the request happens. We need to check what to do while the loading is on.
There was a problem hiding this comment.
Yeah I also need to handle the case where markdown fails to load. Can't use td-message right? I remember you had mentioned not wanting this package to depend on core. Just a simple text message?
There was a problem hiding this comment.
Well, it already has covalent/core as a peerDepedency dont remember why.. so we can use w/e we need i guess..
I would just add a standard empty state message with an error message/icon.
translations are my concern.. but we can figure that out.
There was a problem hiding this comment.
Well we already pass in labels for other items, so we can do something similar.
There was a problem hiding this comment.
Okay I will add error states. Brb.
There was a problem hiding this comment.
Okay added: 0f25bb7
| this.loading = true; | ||
| this._changeDetectorRef.markForCheck(); | ||
|
|
||
| const itemToVerifyWith: IMarkdownNavigatorItem = this.historyStack[0]; |
There was a problem hiding this comment.
shouldnt this be this.historyStack[this.historyStack.length - 1]? or am i misunderstanding this piece of code?
There was a problem hiding this comment.
Also.. not sure if we should allow them to be able to go back while the request happens. We need to check what to do while the loading is on.
Here is where I am trying to check whether the response is still valid, aka the position within the tree has not changed. But I actually should probably check the full stack instead of just the first item or last. 🤦♂
There was a problem hiding this comment.
hahaha, yeah we need to check if the response is valid and also if you still need to assign it.. probably it should be added to children when the response comes back so we dont have to do the request again?
There was a problem hiding this comment.
Okay pushed 9adfd80
So I am not adding it to children, just because for markdown, I am also re-fetching upon revisit. If we do want to avoid re-fetching, we should address it in both cases.
There was a problem hiding this comment.
thats fine, we can keep that as an improvement for later if we dont wanna make this PR bigger as long as we open the issue.
There was a problem hiding this comment.
Opened #1729
christianmemije
force-pushed
the
feat/markdown-nav-children-url
branch
from
4c819e0
to
54046e0
Compare
christianmemije
force-pushed
the
feat/markdown-nav-children-url
branch
from
54046e0
to
0f25bb7
Compare
Description
Adds childrenUrl property to IMarkdownNavigatorItem to enable fetching a remote list of children.
Addresses #1721
Test Steps
General Tests for Every PR
npm run serve:prodstill works.npm run tslintpasses.npm run stylelintpasses.npm testpasses and code coverage is not lower.npm run build:libstill works.Screenshots or link to StackBlitz/Plunker