Skip to content
This repository has been archived by the owner on Jul 24, 2021. It is now read-only.

UI-337 Create dropdown menu to replace DETabPanel in Apps #340

Closed
wants to merge 1 commit into from

Conversation

ash3rz
Copy link
Contributor

@ash3rz ash3rz commented Oct 5, 2018

This doesn't complete UI-337, but gets us ready to add the Communities option by switching to the dropdown.

image

Copy link
Contributor

@yeea yeea left a comment

Choose a reason for hiding this comment

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

LGTM!

I never thought about it before, but is it normal for all the methods to be public?

@ash3rz
Copy link
Contributor Author

ash3rz commented Oct 17, 2018

Thanks for the review!
Which methods? The ones in AppNavigationViewImpl I think are all overriding from the AppNavigationView interface so I believe those must be public. I might have missed some others though

Copy link
Member

@psarando psarando left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:


@Override
public void insert(Widget widget, int index, String name) {
if (!selectionMap.containsKey(widget)) {
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering why not use a regular ComboBox over the SimpleComboBox + this selectionMap, but it does seem this widget lookup here is much simpler than trying to find the widget in a ComboBox list store 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well for one, I totally forgot about the regular ComboBox lol
But if I do remember correctly, I think ComboBox was better suited for a list of items of the same type which all had the same method/property that could be used to extract the display name. This is a combo of Trees but they're not all of the same type (OntologyHierarchy, AppCategory, Group), so I'm not sure that would work. I could be wrong though, it's been a while since I've used one.

@ash3rz
Copy link
Contributor Author

ash3rz commented Oct 19, 2018

Thanks for the reviews!

Since the release is on Tuesday, I think I'll wait to see how far I get with the admin interface for Communities to determine if these should be merged with the release or merged afterwards.

@ash3rz
Copy link
Contributor Author

ash3rz commented Nov 13, 2018

I'm going to close this since we decided to go a different direction for the communities listing in the DE - instead of having Communities be a separate tab/option, they'll be part of our current My Apps workspace tab

@ash3rz ash3rz closed this Nov 13, 2018
@ash3rz ash3rz deleted the ui-337 branch November 19, 2018 17:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants