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

Update UI Screenshots for In-process Script Approval and Change System Time Zone in Jenkins Documentation #7559

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

Conversation

gitclonegaurang
Copy link

Updated outdated UI screenshots for the following pages:

  • In-process Script Approval
  • Change System Time Zone

Screenshots have been replaced with more recent images reflecting the latest UI changes.

Images modified:

  • change-system-timezone-user-defined-timezone-2.png
  • change-system-timezone-user-defined-timezone.png
  • inprocess-script-approval-pipeline.png
  • manage-inprocess-script-approval.png
  • unchecked-groovy-sandbox-on-pipeline.png

@gitclonegaurang gitclonegaurang requested a review from a team as a code owner September 30, 2024 04:16
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Please narrow the new screenshots so that they do not sacrifice readability for users on more narrow screens. Increasing the width of the screenshot by 2x will make it much more difficult for readers of narrower screens

@MarkEWaite
Copy link
Contributor

Please narrow the new screenshots so that they do not sacrifice readability for users on more narrow screens. Increasing the width of the screenshot by 2x will make it much more difficult for readers of narrower screens

My comment above is wrong. The wider screenshot does not sacrifice readability in any significant way. The markup explicitly sets the image width on the HTML page to 800, so it is the same for wide or narrow screens.

For visual comparison:

@gitclonegaurang
Copy link
Author

Thank you for your feedback. I had tested the changes locally using Docker and felt that the updated screenshots were appropriate. After your earlier comment yesterday, I was planning to make adjustments by zooming out and taking new screenshots to decrease the height and width. However, after your latest comment on the preview, I'm now unsure if further changes are necessary.

Could you please clarify if any additional adjustments are still needed, or if the current screenshots are acceptable as they are?

Thanks again for your time and guidance!

@MarkEWaite
Copy link
Contributor

I compared each of the screenshots before and after and I believe that one of the larger screenshots is much more difficult to read than it was before:

content/doc/book/resources/managing/unchecked-groovy-sandbox-on-pipeline.png

Comparisons (will need to scroll downward to find the image):

The wider images do not seem to affect the others nearly as much as that one is affected.

@kmartens27
Copy link
Contributor

kmartens27 commented Oct 2, 2024

Hi @gitclonegaurang, I was looking at the files uploaded and found that they could be compressed a bit further than what they are currently at. In doing so, these two images were reduced by over over 50% and save some space. If you are already running the images through compression, it may be worth checking to see if they can be compressed further without losing quality.
change-system-timezone-user-defined-timezone (1)

This image is a bit larger than what is currently shown and interrupts the text a bit. If possible, would you be able to capture this image on a smaller window so that the syntax box is not as wide? It should re-size dynamically so that the text remains the same size but the overall box should reduce in size and allow for a more fitting screenshot.
unchecked-groovy-sandbox-on-pipeline (1)

@gitclonegaurang
Copy link
Author

Hi @kmartens27,

Thank you for the detailed feedback!

I'll make these adjustments and update the PR as soon as possible.

Thanks again for your review and guidance!

@kmartens27
Copy link
Contributor

@gitclonegaurang I just noticed that you are making these changes from the master branch. In the future, the best practice (as described in our getting started steps) would be to create a new branch based off the master/main so that you can make updates/changes without affecting the main/master branch. Following this workflow prevents any unwanted changes from being associated with your master branch and potentially causing issues in the future.

@kmartens27
Copy link
Contributor

Hi @gitclonegaurang just to share a heads up, another user has completed the screenshot update for the Time Zone documentation, so you can remove those images from this pul request. However, the in-process script approval work should still be updated as previously suggested. Let me know if you have any issues or questions on this!

@gitclonegaurang
Copy link
Author

Alright

@gitclonegaurang
Copy link
Author

Hey @kmartens27 I just observed that the screenshot update for change system zone is not yet updated as mentioned in issue #7527 as upon clicking on the Change System Time Zone the image is same as it was at the time of this issue raised . Do let me know if I have to update the same also.

For in-process script approval I will make the changes asap.
Thanks

@kmartens27
Copy link
Contributor

Hi @gitclonegaurang sorry for the confusion on my part. The link I was thinking of was https://www.jenkins.io/doc/book/using/change-time-zone/ which would be different for what you're working on in this PR. You are correct, please proceed with what you had been doing in the first place. They're very similar URLs and documentation so that's on me. Sorry for the confusion!

@gitclonegaurang
Copy link
Author

No problem I will work on them
Thanks for confirming

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.

3 participants