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

Add link reference to a raw git diff #8

Merged
merged 2 commits into from Aug 31, 2022

Conversation

VladMasarik
Copy link

Summary

  • What issues does the pull request solve?
    As per this issue Backstage Upgrade Helper should provide a Git patch  backstage#11642 it would be nice to have access to a git diff that people can then simply apply with git apply patch.diff.

  • What is the feature? (if applicable)
    This PR adds a link onto the main page that links to a raw diff generated by https://github.com/backstage/upgrade-helper-diff .

  • How did you implement the solution?
    I implemented it as "copy" of UpgradeButton component, and just adjusted it / passed in version variables, plus I used the getDiffURL function to get the relevant raw diff URL.

  • What areas of the website does it impact?
    It Impacts the home page.

Test Plan

I am not sure how to easily create a gif of the change. Here is a picture before you click on the "Show me how to upgrade"
image

Image after you click on the "Show me how to upgrade"
image

The "See raw text diff" link redirects you to a relevant diff

What are the steps to reproduce?

Clone my fork

yarn start

Then select some diffs, and click on the "Show me how to upgrade"

Checklist

  • I tested this thoroughly... or at least I was not able to find a way how to break it
  • I added the documentation in README.md (if needed) ... I still have to add some documentation to the backstage repo docs so that people know why it is there, and how they can use it.

@VladMasarik VladMasarik requested review from a team and vinzscam as code owners June 8, 2022 14:12
@VladMasarik
Copy link
Author

What do you think of the "design"? I have no idea how this applies to the previous design, and I pretty much just put in a button where I thought it would be useful. Should I adjust something?

fromVersion,
toVersion
}) => {
if (fromVersion === '') {
Copy link
Author

Choose a reason for hiding this comment

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

I dont know if this is "enough" in terms of checking whether the button should be shown. AFAIK the fromVersion and toVersion will be always present, so this is protecting only the few cases when the website is being loaded.

Copy link
Member

Choose a reason for hiding this comment

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

good point. I think it's enough for now. There is an open issue for displaying "something" when the diff is empty but it shouldn't be addressed here :)

import { Button as AntdButton } from 'antd'
import { getDiffURL } from '../../utils'

const Container = styled.div`
Copy link
Author

Choose a reason for hiding this comment

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

Both styles are copied from the UpgradeButton, should we somehow adjust them? I think it looks good as is.

@freben
Copy link
Member

freben commented Jun 9, 2022

@vinzscam Wanna take a look at this?

@VladMasarik
Copy link
Author

@vinzscam ping

@VladMasarik
Copy link
Author

@vinzscam pong

@VladMasarik
Copy link
Author

@vinzscam @freben ping

@vinzscam
Copy link
Member

vinzscam commented Aug 3, 2022

Hi @VladMasarik!
I owe you a beer for each ping a didn't answer 🙈

This is great!

What do you think of the "design"? I have no idea how this applies to the previous design, and I pretty much just put in a button where I thought it would be useful. Should I adjust something?

The place where the "diff" button is makes sense. The only issue I see is that it steals some space. I can see this 👇 as an alternative

Screenshot 2022-08-03 at 12 27 45

What do you think?

Signed-off-by: Vladimir Masarik <vladimir.masarik@ef.com>
@VladMasarik
Copy link
Author

@vinzscam I moved the button! Do you have any thoughts?
image

@vinzscam
Copy link
Member

@vinzscam I moved the button! Do you have any thoughts? image

This is great @VladMasarik!
Thank you for your contribution. I will merge asap 🚀

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