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

[KED-3030] Fix for plotly.JSONDataSet icon #684

Merged
merged 10 commits into from
Jan 11, 2022

Conversation

rashidakanchwala
Copy link
Contributor

@rashidakanchwala rashidakanchwala commented Jan 6, 2022

Description

Jira Ticket - https://jira.quantumblack.com/browse/KED-3030

Development notes

Updated shortTypeMapping to also map plotly.JSONDataset to 'plotly' so that correct icon can be rendered

Screenshot 2022-01-06 at 17 08 30

QA notes

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

@rashidakanchwala rashidakanchwala changed the title [KED-3030] [KED-3030] Fix for plotly.JSONDataSet icon Jan 6, 2022
@rashidakanchwala rashidakanchwala self-assigned this Jan 6, 2022
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.

I'm not sure I'm looking at the right thing. When I view our demo site and look at Price Histogram it already has the Plotly icon, doesn't it?

@rashidakanchwala
Copy link
Contributor Author

I'm not sure I'm looking at the right thing. When I view our demo site and look at Price Histogram it already has the Plotly icon, doesn't it?

Hi, it's because the demo site for some reason has Price Histogram as plotly.PlotlyDataSet which always had the plotly icon. It was plotly.JSONDataSet that did not.

Joel's spaceflight tutorial uses JSONDataSet, so you can test using it - https://github.com/datajoely/modular-spaceflights. I have also attached a full screenshot as proof where in the metadata panel you can see the data type as JSONDataSet

@tynandebold
Copy link
Member

I'm not sure I'm looking at the right thing. When I view our demo site and look at Price Histogram it already has the Plotly icon, doesn't it?

Hi, it's because the demo site for some reason has Price Histogram as plotly.PlotlyDataSet which always had the plotly icon. It was plotly.JSONDataSet that did not.

Joel's spaceflight tutorial uses JSONDataSet, so you can test using it - https://github.com/datajoely/modular-spaceflights. I have also attached a full screenshot as proof where in the metadata panel you can see the data type as JSONDataSet

Got it! Thanks :)

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.

Great stuff. Works well for me.

RELEASE.md Outdated
Comment on lines 3 to 9
<!--
Use the sections below to add notes for the next release.
Please follow the established format:
- Keep each note concise - ideally commit title length
- Use present tense (e.g. 'Add new feature')
- Include the ID number for the related PR (or PRs) in parentheses
-->
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be deleted as it's guidelines for the release notes.

limdauto and others added 8 commits January 10, 2022 16:23
Signed-off-by: Rashida Kanchwala <[email protected]>
Signed-off-by: Rashida Kanchwala <[email protected]>
Signed-off-by: Rashida Kanchwala <[email protected]>
* add pencil/edit icon; create run-details modal; open/close of that modal working

* add basic text area in modal

* local creation of an Input component

* create ui folder and add generic textarea and input components

* use text-decoration for active state

* modal gets dynamic data from selected run

* run details modal tests

* rename textarea to input and use that in modal; remove other input component

* test written for input component

* updates based on PR review

* Rashida's PR reviews; hide edit run button when comparison view is on

* fix typos; update general edit run modal behavior

* add runDetails mutation

* remove searchable text; add client to mutation so tests pass

* don't format schema

* Update backend schema

* Fixed mypy error

* fixed mypy2

* fixed pylint error

* update to id from runId in graphql response

* update run_id to id in response -2

* UpdateRunDetails returns Run

* correct response from updateRunDetails mutation; reset mutation on error; use placeholder for notes field

* fix run-selection anomoly

* fix failing RunDetailsModal test

* update mutation response

* use the Modal component from the repo, not the QB UI one

* fix failing RunDetailsModal test

* remove working_directory from circleci

* add working_directory to build_38

* use tmp folder instead of repo

* revert and use arch

* revert by removing working_directory from build_38 and test build again

* clear input on trigger; remove arch from circle ci

Co-authored-by: Rashida Kanchwala <[email protected]>
Signed-off-by: Rashida Kanchwala <[email protected]>
Signed-off-by: Rashida Kanchwala <[email protected]>
Signed-off-by: Rashida Kanchwala <[email protected]>
* updated new data flow diagram with architecture docs

* update architecture docs wording as per suggested

* added in note formatting

* minor wording changes

Signed-off-by: Rashida Kanchwala <[email protected]>
@rashidakanchwala rashidakanchwala force-pushed the KED-3030/bug/fix-plotlyJSONDataSet-icon branch from 7acfc07 to b9d27bf Compare January 10, 2022 16:23
Copy link
Contributor

@studioswong studioswong left a comment

Choose a reason for hiding this comment

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

Works well!

@rashidakanchwala rashidakanchwala merged commit 104ce9b into main Jan 11, 2022
@rashidakanchwala rashidakanchwala deleted the KED-3030/bug/fix-plotlyJSONDataSet-icon branch January 11, 2022 14:26
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.

4 participants