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

Improve REST query naming in navigation #4700

Merged
merged 3 commits into from
Mar 1, 2022

Conversation

aptkingston
Copy link
Member

@aptkingston aptkingston commented Feb 25, 2022

Description

This PR updates the displayed names of REST queries in the navigation.
Described in #4175.

Behaviour

  • Protocol is trimmed
  • Double leading slash is trimmed
  • Trailing slash is trimmed
  • Domain is trimmed if path exists
  • Domain is shown if no path exists

Example of REST datasource:
image

Examples of new naming

Query URL Displayed URL
foo.bar foo.bar
foo.bar/ foo.bar
/foo/bar /foo/bar
//foo.bar foo.bar
//foo.bar/baz/ /baz
http://foo.bar foo.bar
http://foo.bar/ foo.bar
http://foo.bar/baz /baz
{{variable}}/foo/bar/ /foo/bar
{{variable}} {{variable}}
/{{variable}} /{{variable}}
http://foo.{{variable}}.bar/foo/bar/ /foo/bar
https://{{subdomain}}.{{domain}}.com/{{foo}}/bar/{{baz}}/ /{{foo}}/bar/{{baz}}

@codecov-commenter
Copy link

codecov-commenter commented Feb 25, 2022

Codecov Report

Merging #4700 (4304205) into develop (f37a4ee) will not change coverage.
The diff coverage is n/a.

❗ Current head 4304205 differs from pull request most recent head 02c6885. Consider uploading reports for the commit 02c6885 to get more accurate results

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #4700   +/-   ##
========================================
  Coverage    67.91%   67.91%           
========================================
  Files          144      144           
  Lines         4922     4922           
  Branches       761      761           
========================================
  Hits          3343     3343           
  Misses        1105     1105           
  Partials       474      474           

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 2cf0011...02c6885. Read the comment docs.

@@ -109,6 +109,36 @@ export function customQueryIconColor(datasource, query) {
}
}

export function customQueryText(datasource, query) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great to see the example usage of this in the pr description. Could we add unit tests for this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call actually, it's a perfect candidate. Will add some in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Never mind actually - I forgot that we nuked jest from the builder since it was so horribly broken. This is just something we'll need to remember to add in once we've got a working jest setup again.

Copy link
Member

@shogunpurple shogunpurple left a comment

Choose a reason for hiding this comment

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

LGTM! URLs look super clean - love the way it accounts for all different configurations.

@aptkingston aptkingston merged commit 440eb80 into develop Mar 1, 2022
@aptkingston aptkingston deleted the rest-api-endpoint-naming branch March 1, 2022 11:41
@github-actions github-actions bot locked and limited conversation to collaborators Mar 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants