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

Added search in request url to inspector floret #134

Merged
merged 17 commits into from
Feb 18, 2019
Merged

Added search in request url to inspector floret #134

merged 17 commits into from
Feb 18, 2019

Conversation

Shukuyen
Copy link
Collaborator

This pull request adds a search field to the Inspector Floret that allows users to filter the record list by request url. This can be expanded in the future to also filter by response status code or other fields or to highlight the search string in found results.

@brototyp
Copy link
Member

This implementation will filter only over the records that were already requested from the storage and thus are in the records array, right? What do you think about also adding fetching more requests as one scrolls in the filtered results?

@Shukuyen
Copy link
Collaborator Author

Yes, that was the easy way out ;) You are correct, loading more but filtered records would be nice.

…ltered records and improve filtering altogether. All credit goes to Cornelius Horstmann ;)
@Shukuyen
Copy link
Collaborator Author

Updated the PR with our virtual pair programming session results ;) Maybe something for @pstued to look at?

@brototyp
Copy link
Member

Nice pull request. I like it so far. I added some feedback of mine but I would love another review by @pstued as well.

@brototyp
Copy link
Member

1 Error
🚫 Please add a line to the CHANGELOG.md since you changed the SDK.
1 Warning
⚠️ Are there any changes that should be explained in the README.md?
1 Message
📖 You can declare this PR as trivial by adding #trivial to the PR’s title.

Now that I read this, I don't like the "You can declare this PR as trivial by adding #trivial to the PR’s title." message here. It sounds like something the creator of the PR should do, but actually it some kind of documentation. I think we should remove this line for now. What do you think @pstued and @Shukuyen?

@Shukuyen
Copy link
Collaborator Author

Some actionable advice what is expected from the PR creator would be nice. What needs to go into the markdown files, what to do if I don't think my changes are relevant for them and so on. Maybe we can just link the contribution guide and have a section there?

At the moment the changelog says "initial release", should this PR here be mentioned in the changelog anyways? Where and how?

The #trivial thing was something that I interpreted as "Ah, ok, I need to do that to make the errors go away ... maybe?". So from my side: More information for someone who has no idea what is expected :)

@brototyp
Copy link
Member

Some actionable advice what is expected from the PR creator would be nice. What needs to go into the markdown files, what to do if I don't think my changes are relevant for them and so on. Maybe we can just link the contribution guide and have a section there?

At the moment the changelog says "initial release", should this PR here be mentioned in the changelog anyways? Where and how?

I think you are in the hardest position here, because you are the first one to create a PR after the initial release. Sorry about that.

What do you think about that:

  • We change the "Please add a line to the CHANGELOG.md since you changed the SDK." line to something like "Please add a line to the CHANGELOG.md since you changed the SDK. A possible changelog entry for this PR could be * **feature/improvement/bugfix** <title of this PT> [<number of this PR>](<link of this PR>) by @<PR author>. More information here" While all places <like this> are replaced by the actual/correct value.
  • We add a paragraph to the FAQ or Contributing guideline explaining the CHANGELOG in more detail.

(I am listing the FAQ here, because I don't want to put to much in the CONTRIBUTING in order to not scare anyone reading it)

The #trivial thing was something that I interpreted as "Ah, ok, I need to do that to make the errors go away ... maybe?". So from my side: More information for someone who has no idea what is expected :)

How about just removing it altogether (at least for now)? It is actually a way to prevent these warnings and errors to be generated, but I think that case will be quite rare and might just be solvable by any one of us helping there.

.jazzy.yaml Outdated Show resolved Hide resolved
@brototyp-bot
Copy link

brototyp-bot commented Feb 16, 2019

1 Warning
⚠️ Are there any changes that should be explained in the README.md?
1 Message
📖 You can declare this PR as trivial by adding #trivial to the PR’s title.

Generated by 🚫 Danger

Copy link
Member

@brototyp brototyp left a comment

Choose a reason for hiding this comment

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

I left one more comment, which is just a minor feedback, and an entry in the CHANGELOG is needed. Aside of that I am happy to merge.

@Shukuyen
Copy link
Collaborator Author

Should all be done now.

@brototyp
Copy link
Member

Should all be done now.

Nice. The only thing that was left, and why the travis didn't build was a pod install in the example project. I just did that and will merge now. Thanks for the PR!

@brototyp brototyp merged commit 1defc9b into cauliframework:develop Feb 18, 2019
@brototyp brototyp deleted the feature/inspector-search branch February 18, 2019 05:56
@Shukuyen
Copy link
Collaborator Author

Wohooo 🎉 my most scrutinized PR to date and I love it 😁 Thanks for merging.

Sent with GitHawk

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.

4 participants