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

feat: Add element for subset of data to info panel #2622

Open
wants to merge 6 commits into
base: alpha
Choose a base branch
from

Conversation

vardhan0604
Copy link
Contributor

New Pull Request Checklist

Issue Description

Closes: #2610

Approach

TODOs before merging

Copy link

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@vardhan0604
Copy link
Contributor Author

Screen.Recording.2024-10-24.033012.mp4

Hey @mtrezza, I have added a subset element that allows loading another Cloud Code function on demand. I’m not sure about the styles; could you please let me know what changes and features are required after reviewing the current progress?

Copy link

uffizzi-cloud bot commented Oct 23, 2024

Uffizzi Ephemeral Environment deployment-57571

⌚ Updated Oct 23, 2024, 22:03 UTC

☁️ https://app.uffizzi.com/github.com/parse-community/parse-dashboard/pull/2622

📄 View Application Logs etc.

What is Uffizzi? Learn more

@mtrezza
Copy link
Member

mtrezza commented Oct 23, 2024

That is amazing! Just what I had in mind. From a first look I things all that's needed is to refine the design of the section. Let me come up with a mock up... Nice work! Maybe @404-html wants to give this a quick review as well.

@404-html
Copy link
Member

Hi again Gents! 👋

Maybe @404-html wants to give this a quick review as well.

With pleasure!

@@ -95,3 +97,116 @@ export const ButtonElement = ({ item, showNote }) => {
</div>
);
};

export const PanelElement = ({ item, showNote, objectId, depth = 0 }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of introducing another PanelElement I would suggest to extend existing one (in AggregationPanel.js), and adopt it to loading data subset and being used recessively. This way we should avoid code duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried doing in the latest commits, can you please review it?

@vardhan0604
Copy link
Contributor Author

Hi @404-html, I have updated the code as requested. Could you please review it?

@404-html
Copy link
Member

404-html commented Oct 25, 2024

Hi @404-html, I have updated the code as requested. Could you please review it?

Let me wait for Manuel's UI suggestions, and after they will be addressed I will do another review.

@mtrezza
Copy link
Member

mtrezza commented Oct 25, 2024

Not sure why, but for some reason I cannot get the "Show panel" button to show, despite running the same setup like in the previous PR where it worked. @vardhan0604 could you try to rebuild it with the latest commit here and see whether it still works for you?

@vardhan0604
Copy link
Contributor Author

vardhan0604 commented Oct 26, 2024

Not sure why, but for some reason I cannot get the "Show panel" button to show, despite running the same setup like in the previous PR where it worked. @vardhan0604 could you try to rebuild it with the latest commit here and see whether it still works for you?

Yeah, it is working for me. I rebuilt it with the latest commit, and the 'Show panel' button appears as expected. @mtrezza, could you check if parse-dashboard-config.json is configured properly?

@mtrezza
Copy link
Member

mtrezza commented Oct 29, 2024

Still trying to get this to work. Meanwhile I found this:

const Stats = ({ data, classwiseCloudFunctions, className }) => {

Only remotely related to this PR, but maybe we can clean it up at once. Why was classwiseCloudFunctions added to Stats? If I understand it correctly, classwiseCloudFunctions is just for the info panel, but Stats is a completely different feature?

@mtrezza
Copy link
Member

mtrezza commented Oct 29, 2024

#2623 is the reason why the "Show Panel" button isn't showing. Could you take a look?

@@ -591,7 +591,7 @@ export default class DataBrowser extends React.Component {
<ResizableBox
width={this.state.panelWidth}
height={Infinity}
minConstraints={[100, Infinity]}
minConstraints={[400, Infinity]}
Copy link
Member

Choose a reason for hiding this comment

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

Please revert if not required for this PR; the panel should be resizable to min. 100 px width.

panelWidth: 300,
panelWidth: 400,
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, both of them were unintentional changes, i will revert them back in next commit.

@mtrezza
Copy link
Member

mtrezza commented Oct 30, 2024

Here's a suggestion for a slight UI refinement:

Collapsed:

image

Expanded:

image

Notes

  • I've tried to better distinguish the expandable section from a segment header by using a different background color, but defining colors may be a future feature, so maybe it's better to avoid assuming certain color combinations but rather focus on form. Anyway, here's how it would look like in a less-intrusive gray for example:

    image
  • When expanded, the sub-panel is encompassed by a 1px gray line to the left and bottom, to visualize the information hierarchy in the panel.

  • As we can see in the expanded state, there are 2 "Purchases" headlines right below each other, 1 from the expandable sub-panel item, 1 from the title of the segment within the sub-panel. In cases where this is not desired, the title "Purchases" of the first segment can be avoided by simply not specifying a segment title. That feature doesn't exist yet as the title field is a mandatory property of a segment, but it will be easy to add in a future PR.

  • I have used the same design language for expandable sections as we already do in the data browser classes for saved filters:

    • Collapsed:
      image

    • Expanded:
      image

@vardhan0604
Copy link
Contributor Author

#2623 is the reason why the "Show Panel" button isn't showing. Could you take a look?

Yeah sure. I will take a look

@vardhan0604
Copy link
Contributor Author

vardhan0604 commented Nov 2, 2024

#2623 is the reason why the "Show Panel" button isn't showing. Could you take a look?

@mtrezza, should I push the changes here or open a new PR? I’ve already fixed it locally and also added the UI changes.

@mtrezza
Copy link
Member

mtrezza commented Nov 3, 2024

New PR please. It's a fix unrelated to this new feature.

@vardhan0604
Copy link
Contributor Author

vardhan0604 commented Nov 3, 2024

New PR please. It's a fix unrelated to this new feature.

@mtrezza can you review it, I have opened the PR #2627

Also, I’ve implemented the changes according to the UI provided and pushed the changes in this PR. Could you please review and let me know if any additional adjustments are needed.

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.

Add element for subset of data to info panel
3 participants