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

Folder view matches look of job view #424

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

strangelookingnerd
Copy link
Contributor

@strangelookingnerd strangelookingnerd commented Sep 11, 2024

Comparing the view of a folder vs. job I noticed some minor differences this PR aims to fix by adapting the definitions in AbstractFolder/view-index-top.jelly to the core.

Most notable changes:

  • same size of text and icon
  • same display of the "full name" in case a folder is nested

See the following screenshots for comparissions:

Before

Job Folder
grafik grafik
grafik grafik

After

Job Folder
grafik grafik
grafik grafik

One thing that gave me a hard time is the description - there seem to be multiple of those for the folder and the views? Is that intented? I will need some clarification on this before merging to not introduce regressions.

Proposed changelog entries

  • Entry 1: Folder view matches look of job view

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change).
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests

@strangelookingnerd strangelookingnerd requested a review from a team as a code owner September 11, 2024 09:02
@strangelookingnerd
Copy link
Contributor Author

Maybe someone from @jenkinsci/sig-ux could also provide feedback

@timja
Copy link
Member

timja commented Sep 11, 2024

The ideal would be to reuse core and use that instead so that changes automatically get adapted.

If its possible to adapt here / core / both that would be the best solution.

If its not then what you've done here looks good UX wise (I've not tested it manually).

The main changes I saw were:

  • moving description control to the app bar
  • smaller icon
  • full project name being added

jtnord
jtnord previously requested changes Sep 12, 2024
Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

this has removed the system message from folders. (not tested anything else)

@jtnord jtnord requested a review from a team September 12, 2024 09:31
@jtnord
Copy link
Member

jtnord commented Sep 12, 2024

One thing that gave me a hard time is the description - there seem to be multiple of those for the folder and the views? Is that intented?

You mean a folder has a description and a view can also have one, so when using a view for the folder you have potentially 2 conflicting descriptions?
This is the case for Jenkins core (where jenkins has getDescription() returning the system message yet the view for it also has a description. In this case both are shown.
if that is correct / desired behavour I am not sure. Could probably be argued either way (and possibly depends on the view in use), but one thing is for sure, without a request to change it someone will be relying on the existing behaviour and a change without justification will be a terrible regression for them :)

@strangelookingnerd
Copy link
Contributor Author

One thing that gave me a hard time is the description - there seem to be multiple of those for the folder and the views? Is that intented?

You mean a folder has a description and a view can also have one, so when using a view for the folder you have potentially 2 conflicting descriptions? This is the case for Jenkins core (where jenkins has getDescription() returning the system message yet the view for it also has a description. In this case both are shown. if that is correct / desired behavour I am not sure. Could probably be argued either way (and possibly depends on the view in use), but one thing is for sure, without a request to change it someone will be relying on the existing behaviour and a change without justification will be a terrible regression for them :)

As I understand it the AbstractFolder has a description field inherited from AbstractItem - this field can be configured but is only visible on the configuration page and nowhere else.
Then the AbstractFolder has a View that has its own description field which is actually shown in the overview and can only be edited there - but not on the configuration page.

I agree, introducing regressions is not my intent, therefor I left it as-is.

@jtnord
Copy link
Member

jtnord commented Sep 12, 2024

One thing that gave me a hard time is the description - there seem to be multiple of those for the folder and the views? Is that intented?

You mean a folder has a description and a view can also have one, so when using a view for the folder you have potentially 2 conflicting descriptions? This is the case for Jenkins core (where jenkins has getDescription() returning the system message yet the view for it also has a description. In this case both are shown. if that is correct / desired behavour I am not sure. Could probably be argued either way (and possibly depends on the view in use), but one thing is for sure, without a request to change it someone will be relying on the existing behaviour and a change without justification will be a terrible regression for them :)

As I understand it the AbstractFolder has a description field inherited from AbstractItem - this field can be configured but is only visible on the configuration page and nowhere else.

I see it in the folder page.

Then the AbstractFolder has a View that has its own description field which is actually shown in the overview and can only be edited there - but not on the configuration page.

👍

I agree, introducing regressions is not my intent, therefor I left it as-is.

from master I see both descriptions when set

image

@strangelookingnerd
Copy link
Contributor Author

from master I see both descriptions when set

Well, now that's embarassing - sorry for the confusion 🤭
I'll try and fix it.

@strangelookingnerd
Copy link
Contributor Author

Restored the original description beavior in 1a781ba - edited the PR description.

@jtnord
Copy link
Member

jtnord commented Sep 18, 2024

@strangelookingnerd is this complete from your point of view before i merge?

@strangelookingnerd
Copy link
Contributor Author

@strangelookingnerd is this complete from your point of view before i merge?

Yes, thanks for taking care of it 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants