Skip to content

Version creation and update form updates #337

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

Merged
merged 2 commits into from
Apr 24, 2024
Merged

Conversation

agjohnson
Copy link
Contributor

@agjohnson agjohnson requested a review from a team as a code owner April 19, 2024 23:25
@agjohnson agjohnson requested a review from humitos April 19, 2024 23:27
@humitos
Copy link
Member

humitos commented Apr 22, 2024

Note that when using "Fixes:" with bullets, those issues are not closed automatically when the PR is merged, unfortunately 😞

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

This is great!

My suggestion here is about style an rendering only, not functionality. I'd prefer to keep the order "icon + verbose name".

Also, I'd like to align the "Active" element to the right, as shown in #267 (comment)

For reference, this is how the current PR renders locally:

Screenshot_2024-04-22_13-09-24

I'm proposing something like this:

Screenshot_2024-04-22_13-12-00

This matches how we show the "Hidden" and "Default" version on the list of versions.

@agjohnson
Copy link
Contributor Author

agjohnson commented Apr 22, 2024

Also, I'd like to align the "Active" element to the right

The reason I am not doing this is I am matching the project create search view. I decided we should not make a new pattern here.

image

There is an image to the right, we don't have an image for versions however.

I'd prefer to keep the order "icon + verbose name".

The reason I am not doing this is to match the project create search. Also because icon then title pushes the header over to the right, so the description portion is then too far left. This then also needs an icon to push the text right and there also isn't an icon that captures version.identifier -- which can be a commit, tag, or branch.

On the version detail page, I originally did this and it looked too noisy and cluttered. I reflected the change here, and this matches patterns used through this UI better now.

The UI did look like this:

image

The icons usually match each other and it so it looks redundant. We don't use an icon in the header like this anywhere else in the UI also, so I'm not too tied to even keeping it here. I matched this to the search results view after (top line title is verbose name + icon, description line is a code element with the identifier -- again no icon here because there is no explicit description of what the identifier is.

This matches how we show the "Hidden" and "Default" version on the list of versions.

Indeed, but those are listing views. I'm aiming to consolidate the search results views for now.

@humitos
Copy link
Member

humitos commented Apr 23, 2024

There is an image to the right, we don't have an image for versions however.

Gotcha. Can we put the "Active" label in the middle of the row instead so it's aligned between all the versions as shown in the last screenshot of #337 (review)? I think it looks lot better.

Also because icon then title pushes the header over to the right, so the description portion is then too far left

Also note that header and description won't match anyways, since use different fonts (the one from the description is monospaced which make the text a little longer)

@agjohnson
Copy link
Contributor Author

agjohnson commented Apr 23, 2024

Gotcha. Can we put the "Active" label in the middle of the row

Not without headaches, there isn't enough horizontal room for a floating element like this. Project names that aren't short will cause this floating element to appear in odd places -- either push it out of column alignment or push it on to a new line. Really, you need a table for what you want and there isn't enough room.

Placed where it is now, it's intentionally staggered vertically and will look better than when vertical alignment randomly fails.

Also note that header and description won't match anyways, since use different fonts (the one from the description is monospaced which make the text a little longer)

Yeah, longer is fine and expected, it's the unaligned start of the text that is the larger issue. When text doesn't align anywhere is when it looks awkward.

@agjohnson
Copy link
Contributor Author

Also, perhaps another pattern that I might prefer here is expanding labels instead of text labels:

I would want to play around with this as an option here, though would be careful that an icon label was enough to signal to the user that the version is already active -- reducing the text to an icon might not be as clear.

@humitos
Copy link
Member

humitos commented Apr 24, 2024

Looks great! However, it seems pretty low priority to me right now. I'd leave it for when we are polishing the dashboard in the future. I'm fine with what you've done for now.

@agjohnson
Copy link
Contributor Author

I was not planning on it, no. This is a feature addition for later.

@agjohnson agjohnson merged commit add9c4d into main Apr 24, 2024
@agjohnson agjohnson deleted the agj/version-form-updates branch April 24, 2024 18:54
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.

Version creation view updates Filter out "already active" versions when activating a new version
2 participants