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

DYN-1897 API to add extensions to right side bar. #9786

Merged
merged 14 commits into from
Jun 14, 2019

Conversation

reddyashish
Copy link
Contributor

@reddyashish reddyashish commented Jun 10, 2019

Purpose

This PR provides an internal API to add an extension to the right side extensions bar. This API can be called from inside your View Extension class. Inside the "Loaded" function, we can call this method and the window will be added to the right side bar. Tab control mechanism is implemented here.
For example: p.AddToExtensionsSideBar(window);

In the below GIF, I have changed the sample extension class to call this above method instead of calling the window.show() function.

right side bar

Looking forward to any suggestions.

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@QilongTang @mjkkirschner @aparajit-pratap @ColinDayOrg @scottmitchell

@QilongTang QilongTang added the WIP label Jun 10, 2019
@QilongTang
Copy link
Contributor

Great to see progress, do you plan to make a PR to the DynamoSamples repo as well?

@reddyashish
Copy link
Contributor Author

Yes, I will make a PR for that.

@reddyashish reddyashish changed the title DYN-1897 API to add extensions to right side bar. [WIP] DYN-1897 API to add extensions to right side bar. Jun 10, 2019
@mjkkirschner
Copy link
Member

@smangarole will this break any UI automation tests?
@reddyashish If you decide to the DynamoSamples (which I don't think is part of the AC) - please create a 2.3 branch.
Also - this new API needs tests.

@mjkkirschner
Copy link
Member

mjkkirschner commented Jun 10, 2019

@reddyashish I think this looks great! I wonder if we should add a dropdown hamburger

https://fontawesome.com/icons/bars?style=solid

like we do for the workspace tabs to the right?

Can we also add a larger margin between the workspace tab control and the viewExtension tabs?

@reddyashish
Copy link
Contributor Author

The changes we discussed today have been addressed. One thing pending, is to add a test case.

@QilongTang
Copy link
Contributor

QilongTang commented Jun 13, 2019

Looks Solid. At some point I would like to merge our changes together to see what it feel like for the package dependency on the right.

@mjkkirschner
Copy link
Member

Looks good @reddyashish just two comments for you to consider if they worth doing.

@QilongTang
Copy link
Contributor

@reddyashish LGTM. Can you remind me what did we land on Unique ID for each Tab in the end, extension name or?

@reddyashish
Copy link
Contributor Author

The unique id would be the fully qualified type name.(namespace+class name of the extension)

@QilongTang QilongTang added LGTM Looks good to me and removed WIP labels Jun 14, 2019
@QilongTang
Copy link
Contributor

Great, looks solid

@reddyashish reddyashish changed the title [WIP] DYN-1897 API to add extensions to right side bar. DYN-1897 API to add extensions to right side bar. Jun 14, 2019
@reddyashish
Copy link
Contributor Author

Merging this. Will create a todo task to make the API public once the team finalizes it.

@reddyashish reddyashish merged commit 55482f6 into DynamoDS:master Jun 14, 2019
tab.HeaderTemplate = tabDynamic.FindResource("TabHeader") as DataTemplate;

// setting the extension window to the current tab content
tab.Content = contentControl.Content;
Copy link
Member

Choose a reason for hiding this comment

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

@QilongTang @reddyashish - did you guys have a chance to discuss this last week?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I am looking into this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjkkirschner Changing this to set the contentControl object to tab.Content, worked(only for UserControl elements). This would then fail if the user is the creating a window type extension.

I will be making this change in a new PR, but the users will have a constraint of injecting user control elements only.

mjkkirschner pushed a commit that referenced this pull request Aug 3, 2019
* API

* First commit

* second commit

* Comments and some reverts

* Addressing comments and adding event handler to the tabitems list

* making it private

* API change and addressing comments.

* Some more comments

* Adding test

* Adjusting margins

* Adding border property to static resource field

* Adding loggers to console

* Removing the entry from DynamoCoreWPF assembly info.

* Spacing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants