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

Alex krasn/feature share project #344

Merged
merged 38 commits into from
Jul 17, 2020
Merged

Conversation

alex-krasn
Copy link
Contributor

No description provided.

@alex-krasn alex-krasn requested review from tianxiangs and stew-ro June 25, 2020 15:19
@alex-krasn alex-krasn marked this pull request as ready for review June 25, 2020 15:45
.ms-Icon--ChromeRestore:before { @include ms-Icon--ChromeRestore }
.ms-Icon--ChromeMinimize:before { @include ms-Icon--ChromeMinimize }
.ms-Icon--System:before { @include ms-Icon--System }
.ms-Icon--SquareShape:before { @include ms-Icon--SquareShape }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we append to this list instead of replacing it every time. I can't tell what's added or deleted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stew-ro I thought, you told me that we have to replace this file - when we adding icons.

@@ -15,29 +23,21 @@
"unicode": "EE69"
},
{
"name": "Table",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we append to this instead of replacing/rearranging every time. I can't tell what's being added and deleted

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing this icon:
image

If we append instead of replacing/rearranging whole file, catching these errors will be easier

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is the same, you told me that we have to replace this file. I'll add the missing icon though. What is the name of the missing icon?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Contributor

@stew-ro stew-ro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
Doesn't handle invalid "uri"
2.
image

  • It isn't clear that selecting connection and inputting shared project uri are two separate options. I think we should add a separator and two separate headers: "Select connection" ----or---- "Input shared shared project URI"

image

  • Was it decided to put the button here? I expected it to be in the project settings.
  • I think there should be a separator between "share project" and the other items because they're unrelated. Similar to the following:
    image

Copy link
Contributor

@stew-ro stew-ro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Share button should be disabled when project connection is of type "localFileSystem"
image

Copy link
Contributor

@stew-ro stew-ro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to handle empty azure connections
image

Copy link
Contributor

@stew-ro stew-ro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
Can't access input for Shared project URI

Copy link
Contributor

@stew-ro stew-ro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
Can't access project share with my configuration still

image
Not handling invalid input case

Copy link
Contributor

@stew-ro stew-ro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
image

Copy link
Contributor

@stew-ro stew-ro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Showing a project selected from the connection (not the project shared)

<div className={`shared-string-input-container ${this.state.selectedConnection ? "hide" : ""}`}>
<div className="condensed-list-header bg-darker-2 shared-uri-header">
Shared Project String
</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent

<div className="condensed-list-header bg-darker-2 shared-uri-header">
Shared Project String
</div>
{!this.props.connections.length &&
Copy link
Contributor

@stew-ro stew-ro Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to find() if contains cloud projects.

Doesn't show when I have only local connections
image

@@ -133,13 +169,58 @@ export class CloudFilePicker extends React.Component<ICloudFilePickerProps, IClo
const storageProvider = StorageProviderFactory.createFromConnection(this.state.selectedConnection);
const content = await storageProvider.readText(this.state.selectedFile);
this.props.onSubmit(content);
} else if (this.state.pastedUri && this.getSharedProjectConnectionInfo()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of call getSharedProjectConnectionInfo() twice, should assign result as variable

@alex-krasn alex-krasn merged commit d059580 into master Jul 17, 2020
@alex-krasn alex-krasn deleted the alex-krasn/feature-share-project branch July 17, 2020 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Share FOTT project by sharing URL to fott file (without SAS token or security token).
3 participants