Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
docs: Proposal to support multi-source apps in the UI (#17106) #17108
Changes from 1 commit
1320978
6c3baa3
3fedcac
da58d22
6a9e455
3e602f1
88c2009
a0f800c
45a3303
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
Loading complete:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
How many are likely? Just 3, or are 10, and 100 conceivable/likely?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I'm taking advantage of white box's additional top space that is already defined in containers.scss.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.