-
Notifications
You must be signed in to change notification settings - Fork 374
lg editor #87
lg editor #87
Conversation
|
@youyu16 what is the status of this change? it's starting to get stale. |
|
@cwhitten this is pending on the lg parser from botbuilder-js and some changes about files api. I have a local version rebase from master everyday, but import a local version of lg parser and make a work around about how to read lg files is not a proper way. may abandon this and create a new one when ready. |
132a5fd to
62a29e7
Compare
|
@cwhitten please notice that show lg content is only working in this branch. this is just a workaround. |
|
can you please address conflicts that this branch has with master? |
6cbd7ef to
3c5f359
Compare
|
@cwhitten fix the UT and conflict with master. |
|
@youyu16 will you be updating this branch to include writing to the lg file? |
|
@cwhitten yes, I will update this branch include writing to the lg file, I need some time to refactor it using the latest get dialog/lg api which checked in yesterday. |
a12da7c to
87d0d38
Compare
|
lg file auto save should working now |
|
yarn lint is complaining, you might want to run yarn lint --fix |
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.
state.dialogs == lgTemplates?
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.
huh, my bad
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 understand this correct, this api is for updating one lg template, right? then why this is writing the whole content to the 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.
why LG editor affect storageExplorer?
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 a small change to let detail list header stick to the top when you scroll.
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.
let's make it another PR, I guess
boydc2014
left a comment
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.
lgEditor/index.js is empty?
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.
ah, i see, so you merge all lgTemplate to write in here, in client? I think it's better to do this in server, because
- you don't have to pass a lot of data to server
- server have more knowledge, can do the merge better. Server can utilize the knowledge it gains when parsing it to do the job better.
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 definition of api implies that you are updating a single template, but you actually updating the whole lg 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.
is this demo.lg been loaded and used by dialog?
|
I fixed the lint issue and leave some comments @youyu16 |
cwhitten
left a comment
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 getting close. I'd like to see the following:
- Tests! Thank you.
- Merging all LG files into one editing UI. Do we want that? Let's discuss at the next scrum meeting.
- There is a mix of using emotion for css-in-js and inline styles. let's move everything to emotion.
Thanks @cwhitten for bring up #2, this is something i talked with Rachel and Marieke multiple time and still not been convinced that we should hide multiple files behind user. let's talk this in next scrum. |
9b107af to
7ae9b01
Compare
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 should be using IFileStorage
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.
can this get extracted to emotion?
Composer/packages/client/src/pages/content/lg-settings/index.js
Outdated
Show resolved
Hide resolved
Composer/packages/client/src/pages/content/lg-settings/index.js
Outdated
Show resolved
Hide resolved
Composer/packages/client/src/pages/content/lu-settings/index.js
Outdated
Show resolved
Hide resolved
|
Looks good to me, @youyu16 but there is some conflict to resolve |
boydc2014
left a comment
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 resolve the conflict and fix the css issue
4c39ec1 to
1a9ef59
Compare
make a workaroud for show lg content. will change when lg parser is ready
