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

Url shortener #5497

Merged
merged 8 commits into from
Dec 18, 2015
Merged

Url shortener #5497

merged 8 commits into from
Dec 18, 2015

Conversation

BigFunger
Copy link
Contributor

Fixes #1553

This is my first attempt at a working prototype for the url shortener, and I assume that it is going to require changes. I'm looking for input on the implementation.

@BigFunger BigFunger assigned tsullivan and unassigned BigFunger Nov 24, 2015

export default function (server) {
async function updateMetadata(urlId, urlDoc) {
const client = server.plugins.elasticsearch.client;
Copy link
Member

Choose a reason for hiding this comment

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

Can these const client = server.plugins.elasticsearch.client; lines be moved to a single line within the top-level function?

@tsullivan tsullivan assigned BigFunger and unassigned tsullivan Nov 25, 2015
@spalger
Copy link
Contributor

spalger commented Nov 25, 2015

@BigFunger keep in mind that we've standardized on snake_case filenames with #5383

@BigFunger BigFunger assigned tsullivan and unassigned BigFunger Dec 2, 2015
@tsullivan
Copy link
Member

If I create a link to a saved visualization, put that on an iframe somewhere, then delete the visualization, it throws up the "Create a new visualization" screen in the iframe.

screen shot 2015-12-02 at 4 44 34 pm

Not sure if there's a good solution for this. Maybe you could add detail on the warning message for deleting a visualization if there are linked visualizations out there:

screen shot 2015-12-02 at 4 43 34 pm

@tsullivan
Copy link
Member

I don't think the problem regarding an iframe hosting a link to a deleted visualization is a huge deal.

LGTM

@tsullivan
Copy link
Member

I just noticed that when we embed a Search in an iframe, there is a lot of tooling available because the Discover page hasn't really been updated for awareness that it's embedded:

image

@BigFunger BigFunger assigned jbudz and unassigned BigFunger Dec 17, 2015
}
});
} catch (err) {
console.log('Warning: Error updating url metadata', err);
Copy link
Member

Choose a reason for hiding this comment

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

should this use server.log?

@jbudz
Copy link
Member

jbudz commented Dec 17, 2015

Functionally LGTM, made some small comments - passing back.

@jbudz jbudz assigned BigFunger and unassigned jbudz Dec 17, 2015
@BigFunger BigFunger assigned jbudz and unassigned BigFunger Dec 17, 2015
@jbudz
Copy link
Member

jbudz commented Dec 17, 2015

Passing to @panda01 for seconds. Last commit changes LGTM

@jbudz jbudz assigned panda01 and unassigned jbudz Dec 17, 2015
@panda01
Copy link
Contributor

panda01 commented Dec 18, 2015

LGTM!

BigFunger added a commit that referenced this pull request Dec 18, 2015
@BigFunger BigFunger merged commit aad699f into elastic:master Dec 18, 2015
@elasticsearch-bot
Copy link

Jim Unger merged this into the following branches!

Branch Commits
4.x 4831453, efa6210, a4b831c, 8534e67, d77aafd, 683c582, 1b4ada8, 7a8997d

BigFunger added a commit that referenced this pull request Dec 18, 2015
BigFunger added a commit that referenced this pull request Dec 18, 2015
BigFunger added a commit that referenced this pull request Dec 18, 2015
BigFunger added a commit that referenced this pull request Dec 18, 2015
BigFunger added a commit that referenced this pull request Dec 18, 2015
BigFunger added a commit that referenced this pull request Dec 18, 2015
BigFunger added a commit that referenced this pull request Dec 18, 2015
BigFunger added a commit that referenced this pull request Dec 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants