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

[#1485] Make file types selection consistent across panel #1488

Closed
wants to merge 10 commits into from

Conversation

HCY123902
Copy link
Contributor

Part of #1485

Commit message:

Synchronize the file types in zoom panel with the file types in
the summary view

The file types in zoom panel currently do not change actively
when the file type filtering condition in the summary view is
changed.

Making the file types on zoom panel change with the file types
in the summary view can enhance the consistency between
different interface components.

Let's synchronize the zoom panel with the summary view.

@HCY123902 HCY123902 requested a review from a team March 20, 2021 14:37
@gerhean
Copy link
Contributor

gerhean commented Mar 20, 2021

Don't really approve, I believe my fix #1490 is cleaner.

Copy link
Member

@fzdy1914 fzdy1914 left a comment

Choose a reason for hiding this comment

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

PR duplicated with #1490 and #1491 ?

@HCY123902
Copy link
Contributor Author

HCY123902 commented Mar 22, 2021

PR duplicated with #1490 and #1491 ?

There seems to be some slight difference between #1490 and #1488 in the sense that #1490 restores the original implementation of how zoom tab is updated when tabZoomInfo is changed and it restores the original behavior. #1488 synchronizes the change in summary view file types with the file types in the zoom panel immediately after the checkbox in break down by file types are ticked or unticked. This change is made because I interpret that #1485 points out a need to synchronize the file types in the summary view and the file types in the panel responsively. Maybe #1490 can be merged first and then #1488 can be reviewed as a pull request that introduces a new feature. I will try to start another similar pull request to make similar changes in the authorship contribution panel accordingly.

@0blivious
Copy link
Contributor

This disconnection was actually the intended design. The idea is, we wish to separate the tab from the summary view and not updating it until we click to open a new zoom panel. Thus, I am not sure if this PR is necessary.

@damithc may I get your opinion on this change?

@damithc
Copy link
Collaborator

damithc commented Mar 23, 2021

This disconnection was actually the intended design. The idea is, we wish to separate the tab from the summary view and not updating it until we click to open a new zoom panel. Thus, I am not sure if this PR is necessary.

@damithc may I get your opinion on this change?

Are we talking about the chart panel and the code panel? Let's use these terms consistently (those are the names used in the UG https://reposense.org/ug/usingReports.html#report-structure), irrespective of the terms used in the code.

Yes, no need to link them up. The more we link different things, the more susceptible they are to break.

On a related note, it is better to discuss and get the greenlight before implementing a feature. That will reduce the chance of a PR not getting merged down the line.

@Tejas2805
Copy link
Contributor

Tejas2805 commented Mar 23, 2021

This disconnection was actually the intended design. The idea is, we wish to separate the tab from the summary view and not updating it until we click to open a new zoom panel.

If you see the video I have posted for the issue, the error is in the fact, that the commits are removed but the file types are still on display in the Commits Panel when I click to open the Commits Panel. Therefore it should either be that both are not removed or both are removed together. Removal of just one might cause the user to be confused, is my opinion. And that's why I created an issue calling it a bug.

@0blivious
Copy link
Contributor

If you see the video I have posted for the issue, the error is in the fact, that the commits are removed but the file types are still on display in the Commits Panel when I click to open the Commits Panel. Therefore it should either be that both are not removed or both are removed together. Removal of just one might cause the user to be confused, is my opinion. And that's why I created an issue calling it a bug.

The bug should be handled in #1490. This PR looks more like a new feature to me.

@damithc
Copy link
Collaborator

damithc commented Mar 23, 2021

If you see the video I have posted for the issue, the error is in the fact, that the commits are removed but the file types are still on display in the Commits Panel when I click to open the Commits Panel. Therefore it should either be that both are not removed or both are removed together. Removal of just one might cause the user to be confused, is my opinion. And that's why I created an issue calling it a bug.

Yes, that sounds like a bug. Either the panel should be fully disconnected, or fully connected. It's simpler if we keep it disconnected? Is there a significant value in keeping it connected?

@0blivious
Copy link
Contributor

Yes, that sounds like a bug. Either the panel should be fully disconnected, or fully connected. It's simpler if we keep it disconnected? Is there a significant value in keeping it connected?

I don't really see much value in making them connected. Would prefer the fully disconnected one.

@HCY123902 HCY123902 changed the title [#1485] Make file types on zoom panel change with the file types in summary view [#1485] Make file types selection consistent across panel Apr 12, 2021
@fzdy1914
Copy link
Member

Closing the PR as the discussion should have reached a conclusion.

@fzdy1914 fzdy1914 closed this Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants