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

fix: import lg/lu not find all up view #6238

Merged
merged 7 commits into from
Mar 8, 2021
Merged

Conversation

zhixzhan
Copy link
Contributor

@zhixzhan zhixzhan commented Mar 5, 2021

Description

  • locate import file with correct id
  • add intent whitespace on imported project tree item

Task Item

close #6188
close #6189

Screenshots

Mar-05-2021 16-12-06

@hatpick
Copy link
Contributor

hatpick commented Mar 5, 2021

@zhixzhan a couple of notes:

  1. can you change the icon so all of them use the same icon?
  2. I think we just want to show a flat list of LG files here, for example if file1.lg is imported in multiple of other l files, we dont want to see it many times, but one time, can we get UX feedback on this?

@coveralls
Copy link

coveralls commented Mar 5, 2021

Coverage Status

Coverage increased (+0.01%) to 53.446% when pulling 2c0762f on zhixzhan/file-import-resolve into 6238eb4 on main.

@zhixzhan
Copy link
Contributor Author

zhixzhan commented Mar 5, 2021

@zhixzhan a couple of notes:

  1. can you change the icon so all of them use the same icon?

Agree, will update it.

  1. I think we just want to show a flat list of LG files here, for example if file1.lg is imported in multiple of other l files, we dont want to see it many times, but one time, can we get UX feedback on this?

So if several dialog.lg has referred a 'hello.lg'

in dialogA.lg

[import1](hello.lg)

in dialogB.lg

[myHello](hello.lg)

currently will display

- dialogA
     -import1
- dialogB
     -myHello

if we flatten it, it would be

- dialogA
- dialogB
- {name}

here need to choose a {name} to display.

Yes we do need some feedback from designer.

@zhixzhan
Copy link
Contributor Author

zhixzhan commented Mar 8, 2021

Updated the icon and no indent on 'import'

image

@boydc2014 boydc2014 added the 1.4 label Mar 8, 2021
@boydc2014
Copy link
Contributor

@hatpick Agreed about the the ux design here, let's start a thread with designer, for now, i think it's good for us to take this as a fix, what do you think?

@boydc2014 boydc2014 merged commit cfdc8bb into main Mar 8, 2021
@boydc2014 boydc2014 deleted the zhixzhan/file-import-resolve branch March 8, 2021 06:11
@hatpick
Copy link
Contributor

hatpick commented Mar 8, 2021

@boydc2014 I dont think we should show the alias for the import, but we should show the actual imported file name, similar to other languages.

For example, imagine there is a constant.ts file in your typescript project that a bunch of other files import, such as:

file1.ts content:
import * as constants1 from './constant.ts'
file2.ts content
import * as constant2 from './constant.ts'

As you can see in the example above same content from constant.ts has been used with two different aliases, but is that a good UX to show two different items as constant1 and constant2 in the tree view that refer to the same file with the same content?

@srinaath
Copy link
Contributor

srinaath commented Mar 8, 2021

Looping @mewa1024  here.

@mewa1024
Copy link
Contributor

mewa1024 commented Mar 8, 2021

Yes, I think agree with @hatpick above, but I want to check that I understand the suggestion. Back to the original example,
image

this would appear as

bot name
- dialogA
- dialogB
- hello

yes?

@hatpick
Copy link
Contributor

hatpick commented Mar 8, 2021

Correct, that's my suggestion.

@mewa1024
Copy link
Contributor

@sangwoohaan fyi, this is the discussion about imported LG in the all up view. I believe the same behavior is modeled for LU.

lei9444 pushed a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
* resolve correct file

* indent on import item

* lint

* change icon, no indent

Co-authored-by: Dong Lei <donglei@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants