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

Organize Service View Page with Tabs #520

Closed
wants to merge 1 commit into from

Conversation

bai1024
Copy link
Contributor

@bai1024 bai1024 commented Jun 14, 2024

Categorizing the service view page into tabs aims to provide consistency and improve the readability of the page.
Remark:

  • The Member tab will be added once that page is implemented.
  • The project description will be updated until the design is finalized.

Test at local:
image

@bai1024 bai1024 requested a review from a team as a code owner June 14, 2024 06:52
@kfdm
Copy link
Collaborator

kfdm commented Jun 28, 2024

This only seems to update the /service/<id> pages, but most of the same templates are used on the /services/ list pages and the / (home) page.

What probably needs to happen, is to pull out the new changes for the actions dropdown into their own template (similar to the service_header template) and then that can be reused in the various places and cleaned up all at once.

@bai1024
Copy link
Contributor Author

bai1024 commented Jul 2, 2024

What probably needs to happen, is to pull out the new changes for the actions dropdown into their own template (similar to the service_header template) and then that can be reused in the various places and cleaned up all at once.

Updated. Now the action button only used in server/id page so don't need to update other place.

@bai1024 bai1024 closed this Jul 2, 2024
@bai1024 bai1024 reopened this Jul 2, 2024
@bai1024 bai1024 force-pushed the organize-service-view branch 2 times, most recently from ea56ec7 to 704c117 Compare July 24, 2024 06:30
Copy link
Collaborator

@kfdm kfdm left a comment

Choose a reason for hiding this comment

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

Looking better!!!

But there still seems to be a lot of duplicate between service_block.html and service_detail.html.

What I would recommend.

Make a 'panel' for each section. Something like

service_block_panel_projects.inc.html
service_block_panel_rules.inc.html
service_block_panel_notifiers.inc.html

Then from both service_block.html and service_detail you can include the panel. In service_block.html they will be in a list, and in service_detail.html they'll be under each tab.

I think since this PR has gone through a few cycles, there might be some other duplicates in there. I'll keep checking a bit more.

promgen/templates/promgen/service_detail.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@kfdm kfdm left a comment

Choose a reason for hiding this comment

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

Overall it's looking better, but still a few questions on specific lines.

@bai1024 bai1024 force-pushed the organize-service-view branch 2 times, most recently from f114471 to b6f64d6 Compare August 13, 2024 06:53
@bai1024 bai1024 requested a review from kfdm August 14, 2024 01:07
Copy link
Collaborator

@kfdm kfdm left a comment

Choose a reason for hiding this comment

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

Still reviewing, but two things I'm curious about.

Comment on lines 12 to 39
<li>
<a href="">
<form action="{% url 'service-notifier' service.id %}" style="display:inline" method="post" v-pre>{% csrf_token %}
<input type="hidden" name="sender" value="promgen.notification.user">
<input type="hidden" name="value" value="{{request.user.username}}" />
<button style="background:none;border:none;padding:0;">{% translate "Subscribe to Notifications" %}</button>
</form>
</a>
</li>

<hr>

<li role="presentation"><a href="{% urlqs 'audit-list' service=service.id %}">{% translate "Edit History" %}</a></li>
<li role="presentation"><a href="{% urlqs 'alert-list' service=service.name %}">{% translate "Alert History" %}</a></li>

<hr>

<li role="presentation"><a href="{% url 'api:service-rules' name=service.name %}">{% translate "Export Rules" %}</a></li>
<li role="presentation"><a href="{% url 'api:service-targets' name=service.name %}">{% translate "Export Service" %}</a></li>

<hr>

<li role="presentation">
<a href="">
<form method="post" action="{% url 'service-delete' service.id %}" onsubmit="return confirm('{% translate "Delete this service?" %}')" style="display: inline">
{% csrf_token %}
<button style="background:none;border:none;padding:0;color:#d9534f" >{% translate "Delete Service" %}</button>
</form>
</a>
</li>

</ul>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even though <hr> is more semantic, I think using bootstrap's <li role="separator" class="divider"></li> will result in a cleaner layout

Before
before

After
after

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image As you can see in the image attached to the PR description, I have already set the hr tag style, and the original divider style should look like this. . But somehow it didn't work at your local.. I update the hr style again

promgen/templates/promgen/service_action_button_group.html Outdated Show resolved Hide resolved
@bai1024 bai1024 force-pushed the organize-service-view branch 7 times, most recently from 16f1267 to bcb143d Compare August 23, 2024 02:35
@bai1024 bai1024 requested a review from kfdm September 3, 2024 06:12
@kfdm kfdm mentioned this pull request Sep 9, 2024
@kfdm
Copy link
Collaborator

kfdm commented Sep 9, 2024

I've opened up #544 as an attempt to split this into two different PRs. Combining the tab changes and the action button changes in the same PR, made things too difficult to review each time.

In the future, we should try to keep one change to one PR. Also it would help to leave the extra commits around and do a rebase/forcepush when ready for final review. Too many force pushes makes it much harder to review just the changes in isolation.

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.

3 participants