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

[#44] Branches Comparison #45

Merged
merged 21 commits into from
Nov 18, 2022
Merged

[#44] Branches Comparison #45

merged 21 commits into from
Nov 18, 2022

Conversation

lampajr
Copy link
Member

@lampajr lampajr commented Nov 8, 2022

Thank you for submitting this pull request

fixes #44

Improvements

  • [Webpage] Show badges highlighting the branches differences
    • At repository level
    • At global level
  • [CLI] Gather branches comparison data

Copy link
Collaborator

@Ginxo Ginxo left a comment

Choose a reason for hiding this comment

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

Looks good for now! @lampajr

packages/webpage/src/components/current-status/Header.tsx Outdated Show resolved Hide resolved
@lampajr
Copy link
Member Author

lampajr commented Nov 11, 2022

@Ginxo apart from the look and feel which is minimal and should be improved, the rest seems to work (both showing the information and gathering it from github).

I'll try a POC on a custom repository pointing to this branch and I'll post here some screenshots.

Copy link
Collaborator

@Ginxo Ginxo left a comment

Choose a reason for hiding this comment

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

LGTM, just 2 technical changes proposal

@lampajr
Copy link
Member Author

lampajr commented Nov 15, 2022

@Ginxo I fixed the webpage in according to our last call, it can definitely be improved further but the result now is much better then the previous one.

Instead of adding a gray background I preferred to add a divider using the same approach you applied for project's headers. wdyt?

Here some screenshots:
chain-status-1

chain-status-2

chain-status-3

Copy link
Collaborator

@Ginxo Ginxo left a comment

Choose a reason for hiding this comment

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

I LOVE IT!!! mark it as ready for review whenever you feel comfortable with it

packages/webpage/src/components/current-status/Header.tsx Outdated Show resolved Hide resolved
@lampajr
Copy link
Member Author

lampajr commented Nov 16, 2022

I LOVE IT!!! mark it as ready for review whenever you feel comfortable with it
That's great, thanks!

I'll fix the problem of reducing the generated data size and this #45 (comment) point and then I think everything is done

@lampajr lampajr marked this pull request as ready for review November 17, 2022 09:15
@lampajr
Copy link
Member Author

lampajr commented Nov 17, 2022

@Ginxo I think we have to think more about reducing the generated data size.

  • kiegroup-status without branches comparison: ~399 KBs
  • kiegroup-status with 7.67.x and 7.67.x-blue comparison: 1.8 MBs

Maybe considering to keep only the data that we really need.

@lampajr
Copy link
Member Author

lampajr commented Nov 17, 2022

Removing some useless information from branches comparison (i.e., keeping only sha) we reduced by three times the overall size:

  • kiegroup-status without branches comparison: ~399 KBs
  • kiegroup-status with 7.67.x and 7.67.x-blue comparison: ~531 KBs

We can re-include all data for a future persepctive while solving issue #46

@lampajr lampajr requested a review from Ginxo November 17, 2022 10:44
Copy link
Collaborator

@Ginxo Ginxo left a comment

Choose a reason for hiding this comment

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

LGTM, just few proposals

packages/action/src/lib/github/main.js Outdated Show resolved Hide resolved
packages/action/test/support/empty-definition-file.yaml Outdated Show resolved Hide resolved
packages/webpage/src/components/current-status/Header.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@Ginxo Ginxo left a comment

Choose a reason for hiding this comment

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

LGTM, any way new issue open for improving few pending parts
#47

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.

provide new functionallity to compare branches
2 participants