search

jenkins-infra / stats.jenkins.io

Revamped Jenkins Infra Stats Website as a part of GSoC 2024
https://new.stats.jenkins.io
3 stars 4 forks source link

refactor: refactor side/mobile drawer components into one component #54

Closed shlomomdahan closed 2 days ago

shlomomdahan commented 4 days ago

Combines the mobile/side drawer into a single component. This reduces redundancy and repetitive snippets of code.

Vandit1604 commented 3 days ago

Working fine for me on PC and Mobile.

shlomomdahan commented 3 days ago

@krisstern

Yes, because the PR for the plugin versions page is not merged yet. This branch branched off a version of main without the page.

krisstern commented 3 days ago

Hi @shlomomdahan, I would suggest to check out a branch off the PR with plugin versions page if next time this happens again, and leave a note saying there is a dependency based on that PR, as discussed previously. Ideally in a code review you don't want this errorenous link problem to happen, which is misleading.

shlomomdahan commented 2 days ago

Hi @krisstern,

Thank you for pointing that out. I wanted to clarify how the refactor was set up. This PR was branched from the main branch, which currently does not have the "Plugin Versions" feature merged. My understanding was that it's best to keep PRs modular and focused, which is why this PR doesn't include changes from other pending PRs.

The broken link you mentioned will be resolved once the "Plugin Versions" page is merged. Until then, it is expected to be non-functional in this and any other PRs based on the current main branch. Consequently, each PR will be missing certain features from other PRs until they are merged into the main branch.

There is a new PR (#58) that depends on PR (#50), and I made sure to clearly indicate this dependency since those two are linked. However, I don't feel this is the case for the current PR.

This situation raises some confusion on my end regarding the best practices for creating and merging PRs. If possible, could the team provide guidance on the best approach moving forward? @Vandit1604 @lemeurherve @gounthar

Thank you!

lemeurherve commented 2 days ago

(Note: this is just my opinion)

You can create a draft pull request containing all dependencies (in that case the plugin version feature) and this current change so there is a functional branch to test and preview, then create a pull request containing only the current change (here: keep this one), and add links between them stating that the draft is the change with its dependencies for testing & previews but that the PR to review is (in this case) this one.

This is how I (losely) proceed when introducing changes that spread in multiple pull requests.

krisstern commented 2 days ago

Agreeing with @lemeurherve about the approach to add links between PR's. Otherwise, as a reviewer I would have no idea why one feature supposed to have been introduced in an earlier PR is missing in a newer PR. You should at least explain clearly if not basing off from that other PR.

shlomomdahan commented 2 days ago

@krisstern

Understood - going forward will make sure to explain where I am branching from and update whenever possible.

On that note - can we merge this in if it looks OK?

Thanks