Commnets addressed

Signed-off-by: Alberto Gutierrez <aljesusg@gmail.com>
jaegertracing · Nov 15, 2018 · c5b1efc · c5b1efc
1 parent fdf717e
commit c5b1efc
Show file tree

Hide file tree

Showing 12 changed files with 351 additions and 172 deletions.
diff --git a/packages/jaeger-ui/src/components/App/Page.js b/packages/jaeger-ui/src/components/App/Page.js
@@ -15,6 +15,7 @@
 // limitations under the License.
 
 import * as React from 'react';
+import queryString from 'query-string';
 import { Layout } from 'antd';
 import Helmet from 'react-helmet';
 import { connect } from 'react-redux';
@@ -52,17 +53,17 @@ export class PageImpl extends React.Component<Props> {
   }
 
   render() {
-    const isEmbed = this.props.search.includes('embed');
+    const { embed } = queryString.parse(this.props.search);
     return (
       <div>
         <Helmet title="Jaeger UI" />
         <Layout>
-          {!isEmbed && (
+          {embed === undefined && (
             <Header className="Page--topNav">
               <TopNav />
             </Header>
           )}
-          <Content className={ !isEmbed && "Page--content"}>{this.props.children}</Content>
+          <Content className={embed === undefined && 'Page--content'}>{this.props.children}</Content>
         </Layout>
       </div>
     );

diff --git a/packages/jaeger-ui/src/components/SearchTracePage/SearchResults/ResultItem.js b/packages/jaeger-ui/src/components/SearchTracePage/SearchResults/ResultItem.js
@@ -46,10 +46,10 @@ export default class ResultItem extends React.PureComponent<Props> {
 
   render() {
     const {
+      disableComparision,
       durationPercent,
       isInDiffCohort,
       linkTo,
-      disableComparision,
       toggleComparison,
       trace,
     } = this.props;

diff --git a/packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.js b/packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.js
@@ -15,7 +15,7 @@
 // limitations under the License.
 
 import * as React from 'react';
-import { Button, Select } from 'antd';
+import { Select, Button } from 'antd';
 import { Field, reduxForm, formValueSelector } from 'redux-form';
 
 import DiffSelection from './DiffSelection';
@@ -39,9 +39,8 @@ type SearchResultsProps = {
   goToTrace: string => void,
   isEmbed?: boolean,
   hideGraph?: boolean,
-  disableComparision?: boolean,
   loading: boolean,
-  onGoFullClicked: () => void,
+  getSearchURL: () => string,
   maxTraceDuration: number,
   skipMessage?: boolean,
   traces: TraceSummary[],
@@ -111,14 +110,7 @@ export default class SearchResults extends React.PureComponent<SearchResultsProp
         </React.Fragment>
       );
     }
-    const {
-      goToTrace,
-      isEmbed,
-      hideGraph,
-      disableComparision,
-      onGoFullClicked,
-      maxTraceDuration,
-    } = this.props;
+    const { goToTrace, isEmbed, hideGraph, getSearchURL, maxTraceDuration } = this.props;
     const cohortIds = new Set(diffCohort.map(datum => datum.id));
     return (
       <div>
@@ -144,7 +136,12 @@ export default class SearchResults extends React.PureComponent<SearchResultsProp
               <SelectSort />
               {isEmbed && (
                 <label className="ub-right">
-                  <Button className="ub-mr2 ub-flex ub-items-center" onClick={onGoFullClicked}>
+                  <Button
+                    className="ub-mr2 ub-items-center"
+                    icon="export"
+                    target="_blank"
+                    href={getSearchURL()}
+                  >
                     View Results
                   </Button>
                 </label>
@@ -156,17 +153,19 @@ export default class SearchResults extends React.PureComponent<SearchResultsProp
           </div>
         </div>
         <div>
-          {!disableComparision && diffSelection}
+          {diffSelection}
           <ul className="ub-list-reset">
             {traces.map(trace => (
               <li className="ub-my3" key={trace.traceID}>
                 <ResultItem
                   durationPercent={getPercentageOfDuration(trace.duration, maxTraceDuration)}
                   isInDiffCohort={cohortIds.has(trace.traceID)}
-                  linkTo={prefixUrl(isEmbed ? `/trace/${trace.traceID}?embed` : `/trace/${trace.traceID}`)}
+                  linkTo={prefixUrl(
+                    isEmbed ? `/trace/${trace.traceID}?embed&fromSearch` : `/trace/${trace.traceID}`
+                  )}
                   toggleComparison={this.toggleComparison}
                   trace={trace}
-                  disableComparision={disableComparision}
+                  disableComparision={isEmbed}
                 />
               </li>
             ))}

diff --git a/packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.test.js b/packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.test.js
@@ -20,7 +20,6 @@ import * as markers from './index.markers';
 import ResultItem from './ResultItem';
 import ScatterPlot from './ScatterPlot';
 import LoadingIndicator from '../../common/LoadingIndicator';
-import DiffSelection from './DiffSelection';
 
 describe('<SearchResults>', () => {
   let wrapper;
@@ -54,11 +53,6 @@ describe('<SearchResults>', () => {
     expect(wrapper.find(ScatterPlot).length).toBe(0);
   });
 
-  it('hide DiffSelection if queryparam disableComparision', () => {
-    wrapper.setProps({ disableComparision: true });
-    expect(wrapper.find(DiffSelection).length).toBe(0);
-  });
-
   describe('search finished with results', () => {
     it('shows a scatter plot', () => {
       expect(wrapper.find(ScatterPlot).length).toBe(1);

diff --git a/packages/jaeger-ui/src/components/SearchTracePage/index.js b/packages/jaeger-ui/src/components/SearchTracePage/index.js
@@ -64,11 +64,10 @@ export class SearchTracePageImpl extends Component {
     this.props.history.push(prefixUrl(url));
   };
 
-  goFullView = () => {
+  getSearchURL = () => {
     const urlQuery = this.props.query;
     delete urlQuery.embed;
-    const url = `/search?${queryString.stringify(urlQuery)}`;
-    window.open(prefixUrl(url), '_blank');
+    return `/search?${queryString.stringify(urlQuery)}`;
   };
 
   render() {
@@ -85,7 +84,6 @@ export class SearchTracePageImpl extends Component {
       traceResults,
       isEmbed,
       hideGraph,
-      disableComparision,
     } = this.props;
     const hasTraceResults = traceResults && traceResults.length > 0;
     const showErrors = errors && !loadingTraces;
@@ -101,7 +99,7 @@ export class SearchTracePageImpl extends Component {
               </div>
             </Col>
           )}
-          <Col span={18} className="SearchTracePage--column">
+          <Col span={!isEmbed ? 18 : 24} className="SearchTracePage--column">
             {showErrors && (
               <div className="js-test-error-message">
                 <h2>There was an error querying for traces:</h2>
@@ -117,11 +115,10 @@ export class SearchTracePageImpl extends Component {
                 cohortRemoveTrace={cohortRemoveTrace}
                 diffCohort={diffCohort}
                 skipMessage={isHomepage}
-                onGoFullClicked={this.goFullView}
+                getSearchURL={this.getSearchURL}
                 traces={traceResults}
                 isEmbed={isEmbed}
                 hideGraph={hideGraph}
-                disableComparision={disableComparision}
               />
             )}
             {showLogo &&
@@ -145,7 +142,6 @@ SearchTracePageImpl.propTypes = {
   isHomepage: PropTypes.bool,
   isEmbed: PropTypes.bool,
   hideGraph: PropTypes.bool,
-  disableComparision: PropTypes.bool,
   // eslint-disable-next-line react/forbid-prop-types
   traceResults: PropTypes.array,
   diffCohort: PropTypes.array,
@@ -218,9 +214,7 @@ const stateServicesXformer = getLastXformCacher(stateServices => {
 // export to test
 export function mapStateToProps(state) {
   const query = queryString.parse(state.router.location.search);
-  const isEmbed = 'embed' in query;
-  const hideGraph = 'hideGraph' in query;
-  const disableComparision = 'disableComparision' in query;
+  const { embed, hideGraph } = queryString.parse(state.router.location.search);
   const isHomepage = !Object.keys(query).length;
   const { traces, maxDuration, traceError, loadingTraces } = stateTraceXformer(state.trace);
   const diffCohort = stateTraceDiffXformer(state.trace, state.traceDiff);
@@ -237,9 +231,8 @@ export function mapStateToProps(state) {
   return {
     query,
     diffCohort,
-    isEmbed,
-    hideGraph,
-    disableComparision,
+    isEmbed: embed === 'v0',
+    hideGraph: hideGraph !== undefined,
     isHomepage,
     loadingServices,
     loadingTraces,

diff --git a/packages/jaeger-ui/src/components/SearchTracePage/index.test.js b/packages/jaeger-ui/src/components/SearchTracePage/index.test.js
@@ -12,8 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-import prefixUrl from '../../utils/prefix-url';
-
 jest.mock('redux-form', () => {
   function reduxForm() {
     return component => component;
@@ -51,7 +49,6 @@ describe('<SearchTracePage>', () => {
     props = {
       traceResults,
       isEmbed: false,
-      disableComparision: false,
       isHomepage: false,
       loadingServices: false,
       loadingTraces: false,
@@ -114,13 +111,6 @@ describe('<SearchTracePage>', () => {
     wrapper.setProps({ isEmbed: true });
     expect(wrapper.find('.js-test-logo').length).toBe(0);
   });
-
-  it('open a window when goFullView is called', () => {
-    wrapper.setProps({ query: { embed: 'embed', service: 'jaeger-query' } });
-    global.open = jest.fn();
-    wrapper.instance().goFullView();
-    expect(global.open).toBeCalledWith(prefixUrl('/search?service=jaeger-query'), '_blank');
-  });
 });
 
 describe('mapStateToProps()', () => {
@@ -163,7 +153,6 @@ describe('mapStateToProps()', () => {
     expect(rest).toEqual({
       isEmbed: false,
       hideGraph: false,
-      disableComparision: false,
       query: {},
       isHomepage: true,
       // the redux-form `formValueSelector` mock returns `null` for "sortBy"

diff --git a/packages/jaeger-ui/src/components/TracePage/TracePageHeader.css b/packages/jaeger-ui/src/components/TracePage/TracePageHeader.css
@@ -19,26 +19,19 @@ limitations under the License.
   display: flex;
 }
 
-Divider {
-  color: red;
-}
-
 .TracePageHeader--titleRowEmbed {
   align-items: center;
   background-color: #ececec;
   border-bottom: 1px solid #d8d8d8;
   display: flex;
 }
 
-.TracePageHeader--title {
+.TracePageHeader--title,
+TracePageHeader--titleEmbed {
   margin: 0;
   padding: 0.25rem 0.5rem;
 }
 
-.TracePageHeader--titleEmbed {
-  margin: 0;
-  padding: 0.25rem 0.5rem;
-}
 .TracePageHeader--title:hover {
   background: #f5f5f5;
 }