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

Fix/remove tags from panel #1100

Merged
merged 5 commits into from Sep 27, 2022
Merged

Fix/remove tags from panel #1100

merged 5 commits into from Sep 27, 2022

Conversation

Huongg
Copy link
Contributor

@Huongg Huongg commented Sep 27, 2022

Description

Fixes #1092

Development notes

This fix is to only show the tags field from the frontend if it's a task node. We won't remove it from the BE as it will impact the flowchart logic. You can find the conversation here which we discussed specifically about how it would impact the flowchart

If we're all happy with this fix, we can close the PR 1099, which include changes from BE

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Signed-off-by: huongg <huongg1409@gmail.com>
Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

Thank you for working through this! Sorry the original BE change didn't work out as expected but I hope it was educational anyway 🙈

@Huongg
Copy link
Contributor Author

Huongg commented Sep 27, 2022

Thank you for working through this! Sorry the original BE change didn't work out as expected but I hope it was educational anyway 🙈

no worries at all. it was a good learning to make changes from the BE side :)

Copy link
Member

@tynandebold tynandebold 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!

Approving now with a question: do we still have a test that checks if the tags are shown? The two you changed are checking that they're not there. If there isn't a test to check if they do exists maybe you could add one?

Signed-off-by: huongg <huongg1409@gmail.com>
@Huongg
Copy link
Contributor Author

Huongg commented Sep 27, 2022

Looks good!

Approving now with a question: do we still have a test that checks if the tags are shown? The two you changed are checking that they're not there. If there isn't a test to check if they do exists maybe you could add one?

hey @tynandebold yup we already have a test for tags, specifically for TaskNode only

    it('shows the node tags', () => {
      const wrapper = mount({
        nodeId: splitDataTaskNodeId,
        mockMetadata: nodeTask,
      });
      const row = rowByLabel(wrapper, 'Tags:');
      expect(textOf(rowValue(row))).toEqual(['Split']);
    });

RELEASE.md Outdated Show resolved Hide resolved
Co-authored-by: Tynan DeBold <thdebold@gmail.com>
@Huongg Huongg merged commit 5abd014 into main Sep 27, 2022
@Huongg Huongg deleted the fix/remove-tags-from-panel branch September 27, 2022 21:34
@tynandebold tynandebold mentioned this pull request Jan 18, 2023
5 tasks
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.

Remove "Tags" from datasets
3 participants