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

docs: Proposal to support multi-source apps in the UI (#17106) #17108

Merged

Conversation

keithchong
Copy link
Contributor

This is the proposal document for supporting multi-source applications in the UI

Fixes #17106

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@keithchong keithchong requested review from a team as code owners February 6, 2024 23:14
Signed-off-by: Keith Chong <kykchong@redhat.com>
@keithchong keithchong force-pushed the 17106-UiProposalMultiSourceApps branch from d67687c to 1320978 Compare February 7, 2024 23:10
docs/proposals/multiple-sources-for-applications-ui.md Outdated Show resolved Hide resolved
docs/proposals/multiple-sources-for-applications-ui.md Outdated Show resolved Hide resolved
docs/proposals/multiple-sources-for-applications-ui.md Outdated Show resolved Hide resolved
docs/proposals/multiple-sources-for-applications-ui.md Outdated Show resolved Hide resolved
docs/proposals/multiple-sources-for-applications-ui.md Outdated Show resolved Hide resolved
widgets to allow users to cycle (via pagination or combo selector?) through each source. There are API calls to retrieve
the data for each source.

<img height="50%" width="50%" src="images/new-sources-tab.png"/>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of this specific rendering.

  • Generally this specific widget is more commonly used at the bottom of a view (sometimes both top and bottom).
  • I don't have a better proposal at this time.
  • Sometimes drop-downs are used for this purpose, although there are definite downsides.

How many are likely? Just 3, or are 10, and 100 conceivable/likely?

Copy link
Contributor Author

@keithchong keithchong Feb 9, 2024

Choose a reason for hiding this comment

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

Hi @jsoref, I think there will be advantages and disadvantages as well. In the appendix, I talked about collapsible cards where each card houses the source/input parameters. But it can get unwieldy if we try to support N-number of sources.

Maybe it could end up being a combination of collapsible cards and pagination. Note that there is some time required to load the source details. So potentially, we can defer loading until the page is turned. If all cards are in the one tab, then all sources need to be loaded.

From what I was told, from the original multi-source proposal, they discussed 10 sources. But don't know what users will do.

Copy link
Member

Choose a reason for hiding this comment

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

I think I'm slightly more inclined to a vertical list of things on the left that one can click on, akin to: github.com/settings:
image

It's also more or less how IntelliJ and some other things manage...

But, I'm not wed to anything... This is more of a seeking scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I agree. Good to see what scenarios there are and what people suggest. The Argo CD UI doesn't really have a 'representation' like that right now...I think. The closest thing I think is the list view of applications (and also the details list view of an application). The Applications cards represent the Source cards, but they need to be expandable and collapsible. The delete action will be on each card. And the New Source action will be at the top somewhere above the top card. So maybe for consistency, this is the way to go? I don't think it will take much effort to get this up and running.

Yeah, I agree that the IntelliJ (and also the VSCode) vertical view on the left is good, but this change will be more involved.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be difficult to add enough information about the sources in the vertical view without making each item a bigger block with multiple fields. It might be good to have 10 sources per page and a collapsible card / pop-up modal for each source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

StickyTitleForSources

Here I'm taking advantage of white box's additional top space that is already defined in containers.scss.

Copy link
Member

Choose a reason for hiding this comment

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

That's not bad -- although it's kinda hard to read. Could you give it a ring and opacity or something so it's easier to see?

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 is what I was referring to, where if you change the repo URL, or simply change the path within the repo, the input parameters and even the source type can change. So it's probably better to keep these values as separate sections. So perhaps we will need two Edit buttons, like in the screenshot below? As mentioned, for single source apps, these fields are shown in the Summary section, under a different editable section. If you change the values there, the parameters tab will be updated once you visit the tab.

multipleEditableSections

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... Yeah, I think it makes sense to have two edit buttons because they're really very unrelated things

Copy link
Contributor Author

@keithchong keithchong Mar 1, 2024

Choose a reason for hiding this comment

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

The initial PR changes for 'read-only' mode is here. #17275.

A lot of it is refactoring. I think we can make more progress from these changes.

docs/proposals/multiple-sources-for-applications-ui.md Outdated Show resolved Hide resolved
docs/proposals/multiple-sources-for-applications-ui.md Outdated Show resolved Hide resolved
docs/proposals/multiple-sources-for-applications-ui.md Outdated Show resolved Hide resolved
keithchong and others added 7 commits February 9, 2024 15:21
Co-authored-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Keith Chong <kykchong@redhat.com>
Co-authored-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Keith Chong <kykchong@redhat.com>
Co-authored-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Keith Chong <kykchong@redhat.com>
Co-authored-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Keith Chong <kykchong@redhat.com>
Co-authored-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Keith Chong <kykchong@redhat.com>
Co-authored-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Keith Chong <kykchong@redhat.com>
Co-authored-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
Signed-off-by: Keith Chong <kykchong@redhat.com>
Signed-off-by: Keith Chong <kykchong@redhat.com>
Comment on lines +101 to +103
The view should show one source at a time (similar to what the UI is doing now, which only shows one source), but with
widgets to allow users to cycle (via pagination or combo selector?) through each source. There are API calls to retrieve
the data for each source.
Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify the proposal based on what was discussed above, does this make sense?

Suggested change
The view should show one source at a time (similar to what the UI is doing now, which only shows one source), but with
widgets to allow users to cycle (via pagination or combo selector?) through each source. There are API calls to retrieve
the data for each source.
The view should show all the different sources in individual collapsible widgets with pagination to allow users to cycle through each source.
There are API calls to retrieve the data for each source.
When there is only one source is the view, it will be expanded by default. If multiple sources are defined, they will be collapsed and the user will be able to expand them and paginate based on the number of items per page and page number. The default number of item per page should be 3 and saved in the user session.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @agaudreault . Looks good. Just one comment about pagination with 3 items per page. The paginate component has hardcoded values, with 5 items per page as the minimum, then, 10, 15, 20 and All, as choices. How about keeping it at 5 items per page as the minimum, because if we add 3 as a choice (as the simplest change, and ignoring an enhancement to provide custom values), it will change all the pages that use this paginate component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the comment "There are API calls to retrieve the data for each source": I looked into this some more and I think for better performance and usability, it should load the source details 'on demand' when the source section is expanded. Like this:

Just shortly after clicking the expand button:
LoadingOneSource

Loading complete:
LoadedOneSource

Copy link
Member

Choose a reason for hiding this comment

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

LGTM, I thought the default could have been configurable easily. I like the async loading, I was expecting the data to be already fetched, because it is part of the Application Manifest information, but if it needs to load something, async is definitely my preference.

Copy link
Member

@ishitasequeira ishitasequeira left a comment

Choose a reason for hiding this comment

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

LGTM!!

@ishitasequeira
Copy link
Member

@jsoref @ashutosh16 @agaudreault are there any concerns with merging the proposal?

@agaudreault
Copy link
Member

LGTM

Copy link
Member

@jsoref jsoref left a comment

Choose a reason for hiding this comment

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

I can live with it

@crenshaw-dev crenshaw-dev merged commit 4abc992 into argoproj:master Apr 8, 2024
23 checks passed
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.

Proposal: Support multiple sources for an Application in the UI
6 participants