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: added new and cloned panel at the bottom of the page #6993

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

SagarRajput-7
Copy link
Contributor

@SagarRajput-7 SagarRajput-7 commented Jan 31, 2025

Summary

New panel will be added at the bottom, it will also auto adjust itself according to the space available (new row or existing row)


This test suite covers several important scenarios:

  • Empty layout - widget should be placed at origin (0,0)
  • Empty layout with custom dimensions
  • Placing widget next to an existing widget when there's space in the last row
  • Placing widget at bottom when the last row is full
  • Handling multiple rows correctly
  • Handling widgets with different heights

Related Issues / PR's

Screenshots

Screen.Recording.2025-01-31.at.10.56.16.AM.mov
Screen.Recording.2025-01-31.at.11.00.14.AM.mov
image

Affected Areas and Manually Tested Areas


Important

Adds placeWidgetAtBottom function to position new and cloned widgets at the bottom of the layout, with tests for various scenarios.

  • Behavior:
    • Adds placeWidgetAtBottom function in utils.ts to position new widgets at the bottom of the layout.
    • Updates WidgetGraphComponent.tsx and index.tsx to use placeWidgetAtBottom for placing cloned and new widgets.
  • Tests:
    • Adds NewWidget.test.tsx to test placeWidgetAtBottom function for various layout scenarios.
  • Misc:
    • Refactors widget placement logic in utils.ts and utils.ts in hooks/dashboard to use placeWidgetAtBottom.

This description was created by Ellipsis for a7a2f28. It will automatically update as commits are pushed.

@github-actions github-actions bot added enhancement New feature or request docs required labels Jan 31, 2025
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Reviewed everything up to 3a8c8df in 1 minute and 3 seconds

More details
  • Looked at 92 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/container/GridCardLayout/GridCard/WidgetGraphComponent.tsx:135
  • Draft comment:
    Consider extracting the reduce logic for calculating maxY into a utility function to avoid code duplication. This pattern is repeated in multiple files.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code uses a reduce function to calculate maxY, which is a common pattern in the PR. This pattern is repeated in multiple files, and it is a good practice to extract such logic into a utility function to avoid code duplication and improve maintainability.

Workflow ID: wflow_NzFDmu0WkDlOqDlo


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

amlannandy
amlannandy previously approved these changes Jan 31, 2025
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Incremental review on bf8beb0 in 56 seconds

More details
  • Looked at 158 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/container/NewWidget/utils.ts:618
  • Draft comment:
    The y coordinate should be maxY instead of maxY - (widgetHeight || 6) to correctly place the widget at the bottom.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The current code subtracts the widget height to align the new widget with existing widgets in the last row. This makes sense because maxY represents the bottom of the last row (y + height), so we need to subtract the height to align the tops of the widgets. If we used just maxY, the new widget would be placed below the last row instead of aligned with it. The current implementation appears correct.
    The comment author might be thinking that maxY represents the top of the last row rather than the bottom. Or they might have a different mental model of how the grid coordinates work.
    The current implementation correctly aligns widgets in the last row by accounting for widget height. The comment's suggestion would break this alignment.
    The comment should be deleted because it suggests a change that would break the widget alignment functionality. The current implementation is correct.
2. frontend/src/container/GridCardLayout/GridCard/WidgetGraphComponent.tsx:133
  • Draft comment:
    Avoid using inline styles in React components. Move styles to external stylesheets or styled components. This is also applicable at lines 238 and 248 in other files.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. frontend/src/container/NewWidget/index.tsx:364
  • Draft comment:
    Avoid using inline styles in React components. Move styles to external stylesheets or styled components. This is also applicable at lines 238 and 248 in other files.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_GfV1jCQHpCMKGl7S


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Incremental review on 9abeb0c in 32 seconds

More details
  • Looked at 44 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/hooks/dashboard/utils.ts:3
  • Draft comment:
    Ensure that the placeWidgetAtBottom function is correctly implemented to place the widget at the bottom of the layout. This is crucial for the intended functionality of this PR.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statement for placeWidgetAtBottom suggests a new utility function is being used. It's important to verify its implementation to ensure it correctly places the widget at the bottom.
2. frontend/src/hooks/dashboard/utils.ts:3
  • Draft comment:
    Avoid using the component/index.tsx file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. This is applicable to the import from 'container/NewWidget/utils'.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  1. The comment seems to misunderstand the actual import - it's from a utils.ts file, not an index.tsx file. 2. The comment is trying to enforce a file structure rule but is doing so incorrectly in this case. 3. Even if the rule is valid in general, this specific import doesn't violate it. 4. The import is part of the changes in the diff.
    The import could theoretically be from an index.tsx file that re-exports from utils.ts. I can't see the full file structure.
    Even if that were true, the comment is specifically complaining about index.tsx files, which this import statement doesn't directly reference. The comment is making assumptions without evidence.
    The comment should be deleted because it appears to be incorrect about the file structure being used, and even if there was an underlying index.tsx file, the comment isn't providing actionable feedback about the actual changed code.

Workflow ID: wflow_auzzUxLvzqwmvPy7


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

);

// If there's enough space for a 6-width widget
if (maxXInLastRow + 6 <= 12) {
Copy link
Member

Choose a reason for hiding this comment

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

remove this harcoding from 6 to defaultTo(widgetWidth,6)

@srikanthccv
Copy link
Member

@SagarRajput-7 please add tests

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on 85b0f00 in 1 minute and 19 seconds

More details
  • Looked at 110 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/container/NewWidget/__test__/NewWidget.test.tsx:83
  • Draft comment:
    Avoid using hardcoded offsets (like the magic number 2) in tests. Consider deriving the expected Y offset dynamically or documenting its origin, as changes in react-grid-layout behavior may break this test.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. frontend/src/container/NewWidget/__test__/NewWidget.test.tsx:83
  • Draft comment:
    Review: The tests comprehensively cover various scenarios for 'placeWidgetAtBottom'. Consider clarifying the reason for the y offset (2px) in the test comment to link it to react-grid-layout’s behavior.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50%
    None
3. frontend/src/container/NewWidget/index.tsx:371
  • Draft comment:
    Remove the debug console.log before merging to production code.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_Cxr7SE1kcJK4DMtL


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

frontend/src/container/NewWidget/index.tsx Outdated Show resolved Hide resolved
@SagarRajput-7
Copy link
Contributor Author

@SagarRajput-7 please add tests

added 👍

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Incremental review on a7a2f28 in 39 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/container/NewWidget/index.tsx:369
  • Draft comment:
    Good removal of console.log. Consider using environment-specific logging if needed for future debugging.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. frontend/src/container/NewWidget/index.tsx:369
  • Draft comment:
    Good removal of debug logging. Ensure all such logs are removed in production code.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None

Workflow ID: wflow_0m1SBrNjYdoOzxUs


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs required enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants