ui: dashboard links #5704

johncowen · 2019-04-24T15:14:07Z

This PR adds a new {{template-anchor}} component. This component lets you specify a 'href template' in a handlebars like format instead of a normal string href. This template will be interpolated with the contents of a vars="" attribute.

~~hrefs are currently purposefully left blank.~~

~~hrefs use the consul-dashboard-url meta value.~~

hrefs use a UI Setting that you can add on a per user basis via the UI Settings page (see grabs below)

Screens currently use the current styling/widgets - these will be updated across the UI as a whole 🔜

Settings page:

Dashboard link:

banks

@johncowen nice! I might just be not understanding things as my Ember fu is weak but some questions inline

banks · 2019-04-29T13:36:41Z

ui-v2/app/components/templated-anchor.js

+};
+const weak = createWeak();
+
+const templateRe = /{{([A-Za-z.]*)}}/g;


Do you need to escape { and } in JS regexp? I guess not if this works just wondering if it's one of those things where not escaping seems to work but accidentally matches other stuff like how foo.com matches fooZcom? Other examples I've seen of handlebars syntax regexp in JS do escape the opening ones at least?

Not that I'm aware of, it's potentially one of those 'optional' escapes that eslint warns you that it's not needed, will take a look though. First glance its looks like \{ and { both match the same thing.

Ok so, if I've understood correctly, generally you do not need to escape {s unless it's part of a repetition operator (like {1,3}) - but you can if you want. I think I'll put some comments above this at least though. I usually do that with RegExp to make them super clear.

Actually, the more I look at this the more I wonder, I should probably switch the * to a +. Plus, do you know whether we are allowing people to use meta data for interpolation, I was trying to keep the matches pretty restrictive just for our API keys but maybe I should add 0-9 and _- in there?

Key can contain only ASCII chars and no special characters (A-Z a-z 0-9 _ and -)

https://www.consul.io/docs/agent/services.html

What do you think?

Restrictive is great... see reply in main conversation.

banks · 2019-04-29T13:40:06Z

ui-v2/tests/integration/components/templated-anchor-test.js

+      .trim(),
+    'template block text'
+  );
+});


I could be missing something but it looks like this doesn't actually test the variable replacement? Do we know that regexp etc. works?

So these are just the auto generated tests when you use an ember generator to create your stubs. They generally just test the thing you generate can be instantiated.

I've added some integration tests with some basic usecases which cover the regex.

johncowen · 2019-04-29T17:55:59Z

Hey! So I remembered the talk we had last week where we discussed this, and I vaguely remember we briefly discussed being able to use meta keys for interpolation here, so I added those extra characters in as well as adding some basic testing around usage.

banks · 2019-04-29T20:25:27Z

Thanks @johncowen,

and I vaguely remember we briefly discussed being able to use meta keys for interpolation here

I guess this highlights the risk of working on even simple features that weren't well documented just discussed verbally.

The intent of this feature is that we only support interpolating a very small handfull of values from the current context in the UI:

SERVICE_NAME - the name of the service being viewed (e.g. on service listing pages AND service instance pages)
SERVICE_ID - the ID of the service instance being viewed (only on service instance pages)
NODE - the ID(name) of the node on which the service instance is (service instance pages AND node pages)

The whole idea of templating these on the front end came from the original plan that these would be configured centrally (i.e. UI fetches them separately from the service instances themselves).

In moving to pulling the url template from service meta, it basically doesn't make any sense to have this templating done in the UI any more... Consul servers also have all the context the UI does at this point and could just resolve the template themselves in the API which is way more useful.

I'm going to think about this a little more and we can talk about it tomorrow.

johncowen · 2019-04-30T08:32:48Z

Hey Paul!

Thanks for reviewing this, looks like you caught a biggie there! 😅

Yeah for sure, with the talk of being able to interpolate meta data and the suggestion of consul-dashboard-tpl for the name of the meta key - what we have here was what seemed to make sense 🤷‍♂

Don't worry though, if you are going to do all the above in the backend (template interpolation etc), the above PR should still 'work fine' (and I learnt why I don't have to escape {s!). I'm glad we eventually chose the consul-dashboard-url name for the key instead, it means this still 'makes sense'. I should probably remove the extra interpolation here though. Although I'll wait until something is written down to that effect first, so I don't repeat the same mistake.

The intent of this feature is that we only support interpolating a very small handfull of values from the current context in the UI:

SERVICE_NAME - the name of the service being viewed (e.g. on service listing pages AND service instance pages)
SERVICE_ID - the ID of the service instance being viewed (only on service instance pages)
NODE - the ID(name) of the node on which the service instance is (service instance pages AND node pages)

I'm not sure if you are still going with the plan of these strings for interpolation (do we use these values anywhere else), but if you are set on these, then we should probably at least think about making the NODE one into NODE_ID/_NAME (nodes have Node and ID properties, it could probably be confusing if we just used the word NODE. I'm confused now even! ("NODE - the ID(name)" > do you mean the ID or the name? We use Node to refer to the name in the API but you also mention ID here which is a different property in the API) What do you think?

I'm going to think about this a little more and we can talk about it tomorrow.

Cool, gimme a yell when you are about, but we should probably get something written down that you are happy with first so we aren't just discussing these things verbally again!

1. Removes dashboard button from node detail and service instance detail pages 2. Adds very basic settings page to enable input of templatable url 3. Uses new localStorage based template for url interpolation

This pulls in some select details from new design work so that the settings page looks repsectable. Potentially this could clash with another currently open PR. Both this commit and the commits in the other PR look fine on their own, but once merged they 'may' clash somewhere. Decided it was best to add these changes to this PR rather than the other as these changes are a direct result of needing to add new things into the Settings page

johncowen · 2019-05-01T11:58:37Z

This is ready to go again, I also added some screengrabs to the PR description

pearkes · 2019-05-01T17:12:19Z

Looks like Ember tests are failing here ^

pearkes

Just needs CI to pass but otherwise LGTM

johncowen · 2019-05-01T17:50:53Z

Cool, missed a little dependency for a unit test, thanks for spotting! Test are passing again now

This PR adds a new {{template-anchor}} component. This component lets you specify a 'href template' in a handlebars like format instead of a normal string href. This template will be interpolated with the contents of a vars="" attribute. Also contains code to add an extra UI Setting to be able to store a template to be used for this anchor in localStorage

johncowen added the theme/ui Anything related to the UI label Apr 24, 2019

johncowen added this to the 1.5.0 milestone Apr 24, 2019

johncowen marked this pull request as ready for review April 29, 2019 10:41

johncowen requested a review from a team April 29, 2019 10:41

johncowen force-pushed the ui-staging branch from 4cc119e to 1437472 Compare April 29, 2019 11:27

John Cowen added 3 commits April 29, 2019 11:33

ui: Adds templated-anchor component and uses it (hidden for now)

90ff1d8

ui: Adds styles for templated-anchor

3fe55bf

ui: Adds usage of consul-dashboard-url meta property

0cf1317

johncowen force-pushed the feature/ui-dashboard-links branch from 353705f to 0cf1317 Compare April 29, 2019 11:33

johncowen mentioned this pull request Apr 29, 2019

UI: ui-staging merge #5729

Merged

5 tasks

banks reviewed Apr 29, 2019

View reviewed changes

John Cowen added 2 commits April 29, 2019 17:47

ui: Tweak regex to include Meta keys, and don't match empty hbs selector

eabfc38

ui: Add some integration tests for template regex

9913fa5

johncowen requested a review from banks April 29, 2019 17:54

ui: catch malformed paths for template interpolation

fc67f89

ui: Moves dashboard buttons to use localStorage rather than meta data

d8d21c6

1. Removes dashboard button from node detail and service instance detail pages 2. Adds very basic settings page to enable input of templatable url 3. Uses new localStorage based template for url interpolation

johncowen force-pushed the feature/ui-dashboard-links branch from 2e6f520 to d8d21c6 Compare April 30, 2019 18:25

John Cowen added 2 commits May 1, 2019 11:49

ui: Reorganizes layout of settings, disable ENTER press

9172e36

pearkes approved these changes May 1, 2019

View reviewed changes

ui: Add settings as a dependency of the unit test

b3f0ab1

pearkes approved these changes May 1, 2019

View reviewed changes

Merge branch 'ui-staging' into feature/ui-dashboard-links

cde53ba

johncowen merged this pull request into ui-staging May 1, 2019

johncowen deleted the feature/ui-dashboard-links branch May 1, 2019 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: dashboard links #5704

ui: dashboard links #5704

johncowen commented Apr 24, 2019 •

edited

Loading

banks left a comment

banks Apr 29, 2019

johncowen Apr 29, 2019

johncowen Apr 29, 2019 •

edited

Loading

banks Apr 29, 2019

banks Apr 29, 2019

johncowen Apr 29, 2019

johncowen commented Apr 29, 2019 •

edited

Loading

banks commented Apr 29, 2019

johncowen commented Apr 30, 2019

johncowen commented May 1, 2019

pearkes commented May 1, 2019

pearkes left a comment

johncowen commented May 1, 2019

ui: dashboard links #5704

ui: dashboard links #5704

Conversation

johncowen commented Apr 24, 2019 • edited Loading

banks left a comment

Choose a reason for hiding this comment

banks Apr 29, 2019

Choose a reason for hiding this comment

johncowen Apr 29, 2019

Choose a reason for hiding this comment

johncowen Apr 29, 2019 • edited Loading

Choose a reason for hiding this comment

banks Apr 29, 2019

Choose a reason for hiding this comment

banks Apr 29, 2019

Choose a reason for hiding this comment

johncowen Apr 29, 2019

Choose a reason for hiding this comment

johncowen commented Apr 29, 2019 • edited Loading

banks commented Apr 29, 2019

johncowen commented Apr 30, 2019

johncowen commented May 1, 2019

pearkes commented May 1, 2019

pearkes left a comment

Choose a reason for hiding this comment

johncowen commented May 1, 2019

johncowen commented Apr 24, 2019 •

edited

Loading

johncowen Apr 29, 2019 •

edited

Loading

johncowen commented Apr 29, 2019 •

edited

Loading