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

ui: Discovery Chain Continued #6939

Merged
merged 14 commits into from
Dec 17, 2019

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Dec 12, 2019

This PR is a continuation of #6746

Here we add three main things:

  1. We used a small animation engine to animate the lines as they change from node to node instead of just jumping. This code is pretty old, and is likely to be refined in the future. We've had a quick look at using ember-animated here but it doesn't look like it suits our needs right now, and we've had problems with ember and svg before so we decided to use this as its straight forwards and we have full control over it. We've tweaked it a little bit from our original implementation for ember - as far as I can tell when ember updates the dom, its not clear how to keep hold of what was their originally, so we do that in the service here and also clean things up when the component is removed.
  2. Previously as the routing tab wasn't the selected tab when you enter this view, the dom-position helper wouldn't report the correct coordinates for the dom elements as they had no display. This uses IntersectionObserver to helper us to detect when to actually draw the discovery chain (when the tab is selected). We've added this to our dom service and we'll probably reuse this once this is all merged together with the work we are doing on ui: Data Source Component #6821
  3. There is no indication in the API response as to whether a resolver is a redirect or just a normal resolver to a different service/subset. We can guess this if the datacenter or the namespace is different, then its a redirect - if it's not it's 'likely' not to be a redirect but it still could be. More reliable support is waiting on agent: surface some redirect information for the discovery chain compilation results for the ui #6809 In the meantime at least with this guess work we can get his right most of the time.

There are a few other CSS tweaks here just to refine things slightly, and we'll probably add a commit or two on the end of here today as a result of a review of #6746

Oh and we still need to add a 'redirect' icon, right now we are reusing the 'failover' one.

@johncowen johncowen requested a review from a team December 12, 2019 11:57
@johncowen johncowen added the theme/ui Anything related to the UI label Dec 12, 2019
@johncowen johncowen mentioned this pull request Dec 12, 2019
1 task
@codecov
Copy link

codecov bot commented Dec 12, 2019

Codecov Report

Merging #6939 into feature/ui-disco-chain will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@                    Coverage Diff                     @@
##           feature/ui-disco-chain    #6939      +/-   ##
==========================================================
- Coverage                   65.65%   65.61%   -0.04%     
==========================================================
  Files                         443      443              
  Lines                       53312    53312              
==========================================================
- Hits                        35002    34982      -20     
- Misses                      14092    14102      +10     
- Partials                     4218     4228      +10
Impacted Files Coverage Δ
agent/consul/consul_ca_delegate.go 55.55% <0%> (-22.23%) ⬇️
agent/consul/raft_rpc.go 78.72% <0%> (-4.26%) ⬇️
command/catalog/list/nodes/catalog_list_nodes.go 80.34% <0%> (-3.42%) ⬇️
agent/consul/session_ttl.go 85.48% <0%> (-3.23%) ⬇️
api/watch/funcs.go 73.57% <0%> (-3.11%) ⬇️
agent/consul/leader_connect.go 71.02% <0%> (-1.53%) ⬇️
agent/kvs_endpoint.go 75.44% <0%> (-1.2%) ⬇️
agent/consul/rpc.go 78.57% <0%> (-0.76%) ⬇️
agent/consul/connect_ca_endpoint.go 56.52% <0%> (-0.67%) ⬇️
agent/consul/acl_replication.go 75.9% <0%> (-0.67%) ⬇️
... and 9 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 835bd31...3a3dfac. Read the comment docs.

setMethod(method) {
this._method = method;
}
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a quick note here ^ I'll be splitting all this up into separate files at some point

@johncowen johncowen added this to the 1.7.0 milestone Dec 16, 2019
@johncowen johncowen force-pushed the feature/ui-disco-chain branch from 400ff1d to faa2d8d Compare December 17, 2019 15:49
@johncowen johncowen force-pushed the feature/ui-disco-chain-cont branch from b4db3b7 to c62c30d Compare December 17, 2019 15:49
John Cowen added 12 commits December 17, 2019 16:23
1. Use IntersectionObserver so we know when the tab is visible,
otherwise the dom-position helper won't work as the dom elements don't
have any display.
2. Add some base work for animations and use them a little
3. Try to detect if a resolver is a redirect. Right now this works for
datacenters and namespaces, but it can't work for services and subsets -
we are awaiting backend support for doing this properly.
@johncowen johncowen force-pushed the feature/ui-disco-chain branch from faa2d8d to 835bd31 Compare December 17, 2019 16:23
@johncowen johncowen force-pushed the feature/ui-disco-chain-cont branch from c62c30d to 9f9d15b Compare December 17, 2019 16:23
setMethod(method) {
this._method = method;
}
};

Choose a reason for hiding this comment

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

There are all sorts of existing tweening and timing libraries. Why roll your own?

Copy link
Contributor Author

@johncowen johncowen Dec 17, 2019

Choose a reason for hiding this comment

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

This is about 20 year old code I had that I knew was straightforwards and a few lines of code, so I dredged it up and made it work with ember, its basically stuff I already had. Its also based on EventListeners which is a fairly major pattern we use.

I did take a quick look at ember-animated but it didn't seem to work with what I wanted to do and seemed like it took a reasonable amount of time to figure out how to use it properly.

I might switch this out for ember-animated at some point, but I know this one I use here is made to animate based on other things apart from time, which might come in handy at some point.

Also take into account this is all targeted at a beta release.

@johncowen
Copy link
Contributor Author

I was about to add the CSS.Escape thing on the end of here which I completely forgotten about 😬. I'm gonna add it to the end of here.

@johncowen
Copy link
Contributor Author

Ok, I added a commit onto the end of here, I'm gonna merge all this down now. It will sit for a little while longer on ui-staging so feel free to shout if you have anything to add on that last commit. Also the same caveat that we are targeting a beta release here at the moment.

@johncowen johncowen merged this pull request into feature/ui-disco-chain Dec 17, 2019
@johncowen johncowen deleted the feature/ui-disco-chain-cont branch December 17, 2019 18:30
johncowen added a commit that referenced this pull request Dec 17, 2019
1. Add in the things we use for the animations
2 Use IntersectionObserver so we know when the tab is visible,
otherwise the dom-position helper won't work as the dom elements don't
have any display.
3. Add some base work for animations and use them a little
4. Try to detect if a resolver is a redirect. Right now this works for
datacenters and namespaces, but it can't work for services and subsets -
we are awaiting backend support for doing this properly.
5. Add a fake 'this service has no routes' route that says 'Default'
6. redirect icon
7. Add CSS.escape polyfill for Edge
johncowen added a commit that referenced this pull request Dec 18, 2019
* Add data layer for discovery chain (model/adapter/serializer/repo)

* Add routing plus template for routing tab

* Add extra deps - consul-api-double upgrade plus ngraph for graphing

* Add discovery-chain and related components and helpers:

1. discovery-chain to orchestrate/view controller
2. route-card, splitter-card, resolver card to represent the 3 different
node types.
3. route-match helper for easy formatting of route rules
4. dom-position to figure out where things are in order to draw lines
5. svg-curve, simple wrapper around svg's <path d=""> attribute format.
6. data-structs service. This isn't super required but we are using
other data-structures provided by other third party npm modules in other
yet to be merged PRs. All of these types of things will live here for
easy access/injection/changability
7. Some additions to our css-var 'polyfill' for a couple of extra needed
rules

* Related CSS for discovery chain

1. We add a %card base component here, eventually this will go into our
base folder and %stats-card will also use it for a base component.
2. New icon for failovers

* ui: Discovery Chain Continued (#6939)

1. Add in the things we use for the animations
2 Use IntersectionObserver so we know when the tab is visible,
otherwise the dom-position helper won't work as the dom elements don't
have any display.
3. Add some base work for animations and use them a little
4. Try to detect if a resolver is a redirect. Right now this works for
datacenters and namespaces, but it can't work for services and subsets -
we are awaiting backend support for doing this properly.
5. Add a fake 'this service has no routes' route that says 'Default'
6. redirect icon
7. Add CSS.escape polyfill for Edge
@ghost
Copy link

ghost commented Jan 25, 2020

Hey there,

This issue has been automatically locked because it is closed and there hasn't been any activity for at least 30 days.

If you are still experiencing problems, or still have questions, feel free to open a new one 👍.

@ghost ghost locked and limited conversation to collaborators Jan 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants