-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Sync QueryView and QuerySource #4430
Merged
gabrieldutra
merged 11 commits into
migrate-query-source-view-to-react
from
react-query-view
Dec 13, 2019
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
762a388
Sync QueryView and QuerySource
gabrieldutra fae8192
Updates to layout and QueryVisualizationTabs
gabrieldutra 4030334
Add Execute button
gabrieldutra f80c0e1
Remove request to DataSource.types
gabrieldutra e6b25e1
Merge branch 'migrate-query-source-view-to-react' into react-query-view
gabrieldutra d45357b
Change currentVisualizationId prop to be number
gabrieldutra 37c9b46
Fix QueryHeader wrapping behavior for mobile
gabrieldutra 3f1a577
Merge branch 'migrate-query-source-view-to-react' into react-query-view
gabrieldutra 39b1a88
Use old markup
gabrieldutra ef2f4c0
Remove query-page-header.less
gabrieldutra 1230f9f
Remove negatie margin from query tags mobile
gabrieldutra File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -546,7 +546,6 @@ nav .rg-bottom { | |
|
||
.query-tags__mobile { | ||
display: none; | ||
margin: -5px 0 0 0; | ||
padding: 0 0 0 23px; | ||
} | ||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
import React, { useMemo, useState, useEffect } from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import { react2angular } from 'react2angular'; | ||
import Divider from 'antd/lib/divider'; | ||
import Button from 'antd/lib/button'; | ||
import Icon from 'antd/lib/icon'; | ||
import { EditInPlace } from '@/components/EditInPlace'; | ||
import { Parameters } from '@/components/Parameters'; | ||
import { TimeAgo } from '@/components/TimeAgo'; | ||
import { currentUser } from '@/services/auth'; | ||
import QueryPageHeader from './components/QueryPageHeader'; | ||
|
||
import { IMG_ROOT, DataSource } from '@/services/data-source'; | ||
import QueryVisualizationTabs from './components/QueryVisualizationTabs'; | ||
import { EditVisualizationButton } from '@/components/EditVisualizationButton'; | ||
import useQueryResult from '@/lib/hooks/useQueryResult'; | ||
import { pluralize } from '@/filters'; | ||
|
||
function QueryView({ query }) { | ||
const canEdit = useMemo(() => (currentUser.canEdit(query) || query.can_edit), [query]); | ||
const parameters = useMemo(() => query.getParametersDefs(), [query]); | ||
const [dataSource, setDataSource] = useState(); | ||
const queryResult = useMemo(() => query.getQueryResult(), [query]); | ||
const queryResultData = useQueryResult(queryResult); | ||
|
||
useEffect(() => { | ||
DataSource.get({ id: query.data_source_id }).$promise | ||
.then(setDataSource); | ||
}, [query]); | ||
return ( | ||
<div className="query-page-wrapper"> | ||
<div className="container"> | ||
<QueryPageHeader query={query} /> | ||
<div className="query-metadata tiled bg-white p-15"> | ||
<EditInPlace | ||
className="w-100" | ||
value={query.description} | ||
isEditable={canEdit} | ||
editor="textarea" | ||
placeholder="Add description" | ||
ignoreBlanks | ||
/> | ||
<Divider /> | ||
<div className="d-flex flex-wrap"> | ||
<div className="m-r-20 m-b-10"> | ||
<img src={query.user.profile_image_url} className="profile__image_thumb" alt={query.user.name} /> | ||
<strong>{query.user.name}</strong> | ||
{' created '} | ||
<TimeAgo date={query.created_at} /> | ||
</div> | ||
<div className="m-r-20 m-b-10"> | ||
<img src={query.last_modified_by.profile_image_url} className="profile__image_thumb" alt={query.last_modified_by.name} /> | ||
<strong>{query.last_modified_by.name}</strong> | ||
{' updated '} | ||
<TimeAgo date={query.updated_at} /> | ||
</div> | ||
{dataSource && ( | ||
<div className="m-r-20 m-b-10"> | ||
<img src={`${IMG_ROOT}/${dataSource.type}.png`} width="20" alt={dataSource.type} /> | ||
{dataSource.name} | ||
</div> | ||
)} | ||
</div> | ||
</div> | ||
<div className="query-content tiled bg-white p-15 m-t-15"> | ||
{query.hasParameters() && <Parameters parameters={parameters} />} | ||
<QueryVisualizationTabs queryResult={queryResult} visualizations={query.visualizations} /> | ||
<Divider /> | ||
<div className="d-flex align-items-center"> | ||
<EditVisualizationButton /> | ||
<Button className="icon-button hidden-xs"> | ||
<Icon type="ellipsis" rotate={90} /> | ||
</Button> | ||
<div className="flex-fill m-l-10"> | ||
{queryResultData && ( | ||
<span> | ||
<strong>{queryResultData.rows.length}</strong>{' '} | ||
{pluralize('row', queryResultData.rows.length)} | ||
</span> | ||
)} | ||
</div> | ||
<Button type="primary">Execute</Button> | ||
</div> | ||
</div> | ||
</div> | ||
</div> | ||
); | ||
} | ||
|
||
QueryView.propTypes = { query: PropTypes.object.isRequired }; // eslint-disable-line react/forbid-prop-types | ||
|
||
export default function init(ngModule) { | ||
ngModule.component('pageQueryView', react2angular(QueryView)); | ||
|
||
return { | ||
'/queries-react/:queryId': { | ||
template: '<page-query-view query="$resolve.query"></page-query-view>', | ||
reloadOnSearch: false, | ||
resolve: { | ||
query: (Query, $route) => { | ||
'ngInject'; | ||
|
||
return Query.get({ id: $route.current.params.queryId }).$promise; | ||
}, | ||
}, | ||
}, | ||
}; | ||
} | ||
|
||
init.init = true; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
38 changes: 38 additions & 0 deletions
38
client/app/pages/queries/components/QueryVisualizationTabs.jsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import { find, orderBy } from 'lodash'; | ||
import Tabs from 'antd/lib/tabs'; | ||
import { VisualizationRenderer } from '@/visualizations/VisualizationRenderer'; | ||
import Button from 'antd/lib/button'; | ||
|
||
const { TabPane } = Tabs; | ||
|
||
export default function QueryVisualizationTabs({ visualizations, queryResult, currentVisualizationId }) { | ||
const tabsProps = {}; | ||
if (find(visualizations, { id: currentVisualizationId })) { | ||
tabsProps.activeKey = `${currentVisualizationId}`; | ||
} | ||
|
||
const orderedVisualizations = orderBy(visualizations, ['id']); | ||
|
||
return ( | ||
<Tabs {...tabsProps} tabBarExtraContent={(<Button><i className="fa fa-plus m-r-5" />New Visualization</Button>)}> | ||
{orderedVisualizations.map(visualization => ( | ||
<TabPane key={`${visualization.id}`} tab={visualization.name}> | ||
<VisualizationRenderer | ||
visualization={visualization} | ||
queryResult={queryResult} | ||
context="query" | ||
/> | ||
</TabPane> | ||
))} | ||
</Tabs> | ||
); | ||
} | ||
|
||
QueryVisualizationTabs.propTypes = { | ||
queryResult: PropTypes.object, // eslint-disable-line react/forbid-prop-types | ||
visualizations: PropTypes.arrayOf(PropTypes.object), | ||
currentVisualizationId: PropTypes.number, | ||
}; | ||
QueryVisualizationTabs.defaultProps = { queryResult: null, visualizations: [], currentVisualizationId: null }; |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed some difference between old and new header version: when query does not have tags, tags control should stay on the same line with query name (see screenshots). Seems this style is related to that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems it's better to keep markup from base branch, unless you have some better UX ideas for this component. Also, if you think that button group does not fit here - feel free to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wraps in the older version too, just take more for it to happen 🙂. That's due to the existing conflict with
.page-title { display: block }
, fixed in 37c9b46There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now layout is good, but still there some differences:
existing
react
Notice: favorites control should be on the same line with query name, too much space between query name and tags, and tags should go under buttons on right side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, in the original there are two
QueryTagsControl
components, one for the desktop position and another for the mobile one 😪There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually feel css is more reliable than js when it can be used, do you have any advantages in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case the only advantage is that tags component will be rendered only once (+markup will simplified a bit). CSS Grid is the perfect solution, but it still is not supported good enough. For JS, there is a
matchMedia
API which is 100% supported in all our target browsers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW you meant to use a hook, right? (like this one)
It's an interesting hook to have anyway, I tried to use it once I wanted to check in JS if the browser was in Percy,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, something like that 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That hook is not hard to be applied in this context:
But I prefer keeping it in css for now as I don't want to break the angular page yet (we can reconsider when cleaning angular stuff)