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

Update SchemaFrame styling #1013

Merged
merged 2 commits into from
Dec 9, 2019
Merged

Update SchemaFrame styling #1013

merged 2 commits into from
Dec 9, 2019

Conversation

nglgzz
Copy link
Member

@nglgzz nglgzz commented Dec 5, 2019

Before:
before
After:
after

Also:

  • fixed some typos
  • renamed the default export in browser/modules/Docs/Docs.jsx to Docs as it's never imported as Guides

- fixed some typos
- renamed default export to Docs as it's never imported as Guides
@nglgzz nglgzz requested a review from oskarhane December 5, 2019 14:12
Copy link
Member

@oskarhane oskarhane left a comment

Choose a reason for hiding this comment

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

Great to see screenshots in the PR 👍

Overall a very good change, just a phew comments on versioning the suggested procedure and the text around it.

@@ -24,7 +24,7 @@ import Directives from 'browser-components/Directives'
import Carousel from '../Carousel/Carousel'
import Slide from '../Carousel/Slide'

export default class Guides extends Component {
export default class Docs extends Component {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -45,7 +45,7 @@ const HelpFrame = ({ frame }) => {
const { title, subtitle } = chapter
let { content } = chapter

// The commands topic is a special case that uses dymaic data
// The commands topic is a special case that uses dynamic data
Copy link
Member

Choose a reason for hiding this comment

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

👍

<br />
<p className='lead'>Execute the following query for more information</p>
<figure>
<pre className='code runnable'>CALL db.schema.visualization</pre>
Copy link
Member

Choose a reason for hiding this comment

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

neo4j-browser needs (at the time of writing this) to support Neo4j 3.4+, and db.schema.visualization does not exist in Neo4j 3.4. There it's named db.schema(), so you need to do a version check here.

@@ -47,7 +47,7 @@ export const CLUSTER_CYPHER_REQUEST = NAME + '/CLUSTER_REQUEST'
export const FORCE_CHANGE_PASSWORD = NAME + '/FORCE_CHANGE_PASSWORD'

// Helpers
const queryAndResovle = async (driver, action, host, useDb = {}) => {
const queryAndResolve = async (driver, action, host, useDb = {}) => {
Copy link
Member

Choose a reason for hiding this comment

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

😱

<SchemaTable name='Indexes' content={indexes} />
<SchemaTable name='Constraints' content={constraints} />
<br />
<p className='lead'>Execute the following query for more information</p>
Copy link
Member

Choose a reason for hiding this comment

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

The links doesn't really show more information about the indexes and constraints, but rather semi-related info.
I would probably rewrite this to be something in the lines of: Execute the following command to visualize what's related, and how (we use that phrase in the sidebar "Saved scripts" -> "Data profiling").

@@ -32,7 +35,7 @@ import {
StyledBodyTr,
StyledTd
} from 'browser-components/DataTables'
import { Directives } from 'browser-components/Directives'
import Directives from 'browser-components/Directives'
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why, but after I connected SchemaFrame to redux, the onItemClick function from Directives wouldn't work unless I imported the connected version.

@nglgzz nglgzz requested a review from oskarhane December 6, 2019 14:54
Copy link
Member

@oskarhane oskarhane 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, merge on green tests! 🎉

Comment on lines +104 to +119
test('SchemaFrame renders correct suggestion for Neo4j > 3.4', () => {
const indexResult = { records: [] }
const { getByText } = renderWithRedux(
<SchemaFrame indexes={indexResult} neo4jVersion={'3.5.1'} />
)

expect(getByText('CALL db.schema.visualization')).not.toBeNull()
})

test('SchemaFrame renders correct suggestion for Neo4j <= 3.4', () => {
const indexResult = { records: [] }
const { getByText } = renderWithRedux(
<SchemaFrame indexes={indexResult} neo4jVersion={'3.4.1'} />
)

expect(getByText('CALL db.schema()')).not.toBeNull()
Copy link
Member

Choose a reason for hiding this comment

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

Great to see tests 👍

@nglgzz nglgzz merged commit 481720a into neo4j:master Dec 9, 2019
@nglgzz nglgzz deleted the schema-frame branch December 9, 2019 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants