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

Use Ant Design instead of Semantic UI #169

Merged
merged 18 commits into from
Jan 29, 2018
Merged

Use Ant Design instead of Semantic UI #169

merged 18 commits into from
Jan 29, 2018

Conversation

tiffon
Copy link
Member

@tiffon tiffon commented Jan 17, 2018

Fix #165. Fix #164. Fix #69.

Note: The blue highlights in the search form in some of the screenshots are outdated. Please refer to the screenshots marked with "†"

Screenshot of the search page showing the custom menu dropdown.
ant-00-search

Service and operation dropdowns now use react react-virtualized-select and are able to show the full list of options.
01--ant-01-service-virtual-select

Hint for tags input.
ant-02-tags-hint

Hint for start date and time input.
ant-03-start-hint

No results.
01--ant-04-no-results

Result list, with a hovered item.
ant-05-results

Trace detail view.
ant-06-trace

Trace name is clickable to collapse the mini-map and overview information.
ant-07-trace-name-hover

Collapsed view.
ant-08-trace-slim

Keyboard shortcut info.
ant-09-kbd-info

Parent spans can be collapsed.
ant-10-parent-hover

A collapsed parent span.
ant-11-parent-collapsed

A span detail row.
ant-12-detail-row

Various details expanded.
ant-13-details-expanded

The force directed graph of the services.
ant-14-force-graph

The DAG.
ant-15-dag

Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
@ghost ghost assigned tiffon Jan 17, 2018
@ghost ghost added the review label Jan 17, 2018
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
Signed-off-by: Joe Farro <joef@uber.com>
@codecov
Copy link

codecov bot commented Jan 17, 2018

Codecov Report

Merging #169 into master will decrease coverage by 1.79%.
The diff coverage is 68.38%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #169     +/-   ##
=========================================
- Coverage   92.67%   90.88%   -1.8%     
=========================================
  Files          85       93      +8     
  Lines        1884     1930     +46     
  Branches      368      383     +15     
=========================================
+ Hits         1746     1754      +8     
- Misses        125      153     +28     
- Partials       13       23     +10
Impacted Files Coverage Δ
src/components/App/index.js 100% <ø> (ø) ⬆️
...nents/TracePage/TraceTimelineViewer/TimelineRow.js 100% <ø> (ø) ⬆️
src/components/TracePage/SpanGraph/index.js 76.92% <ø> (ø) ⬆️
...e/TraceTimelineViewer/SpanDetail/KeyValuesTable.js 77.77% <ø> (ø) ⬆️
src/components/App/NotFound.js 0% <ø> (ø) ⬆️
src/utils/sort.js 100% <ø> (ø) ⬆️
...onents/TracePage/TraceTimelineViewer/SpanBarRow.js 80% <ø> (ø) ⬆️
...nts/TracePage/TraceTimelineViewer/SpanDetailRow.js 100% <ø> (ø) ⬆️
...ge/TraceTimelineViewer/SpanDetail/AccordianLogs.js 100% <ø> (ø) ⬆️
.../components/TracePage/TraceTimelineViewer/index.js 50% <ø> (ø) ⬆️
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da464e5...d02bbe4. Read the comment docs.

Signed-off-by: Joe Farro <joef@uber.com>
Copy link
Member

@saminzadeh saminzadeh left a comment

Choose a reason for hiding this comment

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

Should we change the light blue colors (eg on the dropdown, out of buttons) to the turquoise that is used throughout the page?

Signed-off-by: Joe Farro <joef@uber.com>
@ekelleyv
Copy link

ekelleyv commented Jan 19, 2018

Caveat that my comments are entirely based on the screenshots posted, but just wanted to share a couple of thoughts. Let me know if this is worth discussing somewhere else rather than this PR.

It may be helpful to test some of these designs with applications that have many internal spans. The hot rod application doesn't have too many levels of indentation and uses several different "application tracers" resulting in spans that are more visually legible. Here is a short segment from a trace in our infra::

image

Since everything is in the same service and there are several levels of indentation, it is very hard to see what elements are children of the others.

A couple suggestions:

  1. Add guidelines down each indentation level like code editors do for whitespace
    image

  2. When a parent is hovered, every element that is a child of that element should have some sort of hover state. At the very least, the first and last child should have some marker. I would suggest just extending that hover state down to cover its children (mockup based on posted screenshots):
    image

Aside from that, the screenshots posted here look great.

@tiffon
Copy link
Member Author

tiffon commented Jan 21, 2018

@ekelleyv Thanks for the feedback and suggestion! We've discussed guidelines, internally, but have yet to create a ticket for it. I'll add one now. (Edit: The ticket: #172)

@saminzadeh The color change sounds good to me. 👍

Signed-off-by: Joe Farro <joef@uber.com>
Copy link
Member

@saminzadeh saminzadeh left a comment

Choose a reason for hiding this comment

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

Overall looks good to me given the changes are mostly style and AntD components. Made some small comments about BassCSS.

Also, as mentioned in [previous discussions] (#104), is it worth starting to version the UI. This seems to be pretty big theme change

}
if (error) {
return <NotFound error={error} />;
return <ErrorMessage className="u-space" error={error} />;
Copy link
Member

Choose a reason for hiding this comment

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

Curious what u-space does? Is it a utility?

I notice that we removed Basscss. IMO, I think we should use basscss as a CSS utility library since its small (~2kb), low on side effects and documented.

@@ -0,0 +1,162 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

See common above, should we use Basscss? Believe it will help with developer productivity since its OSS.

Signed-off-by: Joe Farro <joef@uber.com>
@tiffon
Copy link
Member Author

tiffon commented Jan 29, 2018

@saminzadeh Thanks for the suggestion re basscss. I prefer the util css class names to be prepended with u-*, so I forked basscss ((u-basscss)[https://github.com/tiffon/u-basscss]). I also made it so sub-sets of the package can be imported, instead of only the full package.

@tiffon tiffon merged commit b7a3e74 into master Jan 29, 2018
@ghost ghost removed the review label Jan 29, 2018
@yurishkuro yurishkuro deleted the ant-design branch January 29, 2020 15:06
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
* Shift top nav to ant.design

Signed-off-by: Joe Farro <joef@uber.com>

* Use the better string comparator

Signed-off-by: Joe Farro <joef@uber.com>

* Search uses Ant - flow, tests, cleanup are TODO

Signed-off-by: Joe Farro <joef@uber.com>

* Highlight currently active menu option

Signed-off-by: Joe Farro <joef@uber.com>

* Using Ant for all pages, flow and tests are TODO

Signed-off-by: Joe Farro <joef@uber.com>

* Fix bug with text ellipsis in FF

Signed-off-by: Joe Farro <joef@uber.com>

* Fix flow after moving to ant design

Signed-off-by: Joe Farro <joef@uber.com>

* Unit tests passing after shift to ant design

Signed-off-by: Joe Farro <joef@uber.com>

* Fix loss of focus on first change in search form

Signed-off-by: Joe Farro <joef@uber.com>

* Search page aesthetic tweaks

Signed-off-by: Joe Farro <joef@uber.com>

* Remove basscss and semantic-ui.

Signed-off-by: Joe Farro <joef@uber.com>

* Upgrade react-scripts to fix failing unit tests

Signed-off-by: Joe Farro <joef@uber.com>

* Only bundle icons that are used from react-icons

Signed-off-by: Joe Farro <joef@uber.com>

* Misc cleanup

Signed-off-by: Joe Farro <joef@uber.com>

* Fix search form error when deselecting a service

Signed-off-by: Joe Farro <joef@uber.com>

* Adjust the ant theme to the Jaeger teal color

Signed-off-by: Joe Farro <joef@uber.com>

* Swith to using a variant of basscss (u-basscss)

Signed-off-by: Joe Farro <joef@uber.com>

Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
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.

3 participants