-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Dev UI - ArC - add interceptors page #18967
Conversation
mkouba
commented
Jul 23, 2021
- also display associated interceptors on the "beans" page
This reminds me that we should remove unused interceptors as well... I'll try to prepare a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! I added a couple suggestions
extensions/arc/deployment/src/main/resources/dev-templates/beans.html
Outdated
Show resolved
Hide resolved
{#title}Interceptors{/title} | ||
{#body} | ||
<div class="alert alert-primary alert-dismissible fade show" role="alert" data-timer="30000"> | ||
Interceptors are sorted by the interceptor class in ascending order. However, application interceptors go first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it make sense to sort by priorioty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that this would make a lot of sense, but I think that showing application interceptors more prominently than platform/external ones also makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe they should instead have a different color or some marker?
I am definitely no UI expert, so I have no idea what the best representation should be...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So right now we use isApplicationClass
and then interceptorClass
name. We could probably change this to isApplicationClass
and then priority
name and then interceptorClass
name...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe they should instead have a different color or some marker?
They do - application classes are links that can be opened in your IDE... except it does not work for me ;-).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have never been able to test Eclipse for this. A little while ago, @FroMage did and it worked for him
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 279425d
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 integration-tests/devmode✖
📦 integration-tests/main✖
⚙️ JVM Tests - JDK 16 #📦 integration-tests/devmode✖
|
- also display associated interceptors on the "beans" page
279425d
to
c2e35b3
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building c2e35b3
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 Windows #📦 extensions/resteasy-reactive/jaxrs-client-reactive/deployment✖
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't comment much on the HTML templates, but overall looks fine.
I'm going to merge this PR because the Native Tests - Data1 CI job failed with:
|