Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Status page certificate expiry #3357

Merged
merged 8 commits into from
Jul 31, 2023
Merged

Status page certificate expiry #3357

merged 8 commits into from
Jul 31, 2023

Conversation

tarun7singh
Copy link
Contributor

@tarun7singh tarun7singh commented Jul 4, 2023 •
edited
Loading

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Fixes #2451
Fixes #3456
Fixes #2522

Type of change

  • User interface (UI)
  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings

Screenshots (if any)

Screenshot 2023-07-04 at 7 17 16 PM

@tarun7singh
Copy link
Contributor Author

@louislam This pull request is ready fo review, let me know if we need any style changes on how expiry is shown on the status page

CommanderStorm
CommanderStorm requested changes Jul 5, 2023
View reviewed changes
server/model/status_page.js Outdated Show resolved Hide resolved
src/pages/StatusPage.vue Outdated Show resolved Hide resolved
src/components/PublicGroupList.vue Outdated Show resolved Hide resolved
src/components/PublicGroupList.vue Outdated Show resolved Hide resolved
src/components/PublicGroupList.vue Outdated Show resolved Hide resolved
src/components/PublicGroupList.vue Outdated
@@ -60,6 +60,7 @@
@click="$refs.monitorSettingDialog.show(group, monitor)"
/>
</span>
<p v-if="showCertificateExpiry" class="item-name"> Expiry: {{ formatExpiry(monitor) }} </p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Disclaimer: I am not a designer => feel free to overrule
From a design perspective, I don't like how this looks like a regular title (a user would not be able to pick up, that this is an expiry notice).

What do you think: Could we move this to a subtle or tag and still keep the current design?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should i try showing it as pill like the uptime percetage, with colors for showing how soon it will be expiring?
My reason for avoiding that was to not clutter the ui.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, a pill is what I meant with tag. This would be one option.

Copy link
Contributor Author

@tarun7singh tarun7singh Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Here's the pill/tags based design i came up with, which one do you prefer?

  1. Expiry inside the pill
    Screenshot 2023-07-06 at 1 36 50 PM

  2. Expiry outside the pill
    Screenshot 2023-07-06 at 1 35 46 PM

I think second option looks much better but name of the monitor is getting confusing.

Color is green for expiry of more than 7 days
Color is red for everything else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@louislam Do you have any preference out of the above two designs?

src/components/PublicGroupList.vue Outdated Show resolved Hide resolved
@louislam louislam added this to the 1.23.0 milestone Jul 8, 2023
CommanderStorm
CommanderStorm approved these changes Jul 14, 2023
View reviewed changes
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

My concerns have been addressed. (if you mark them as resolved, they are not shown any more ^^)

I (not a desiger/bdfl => this requires Louis vote) am unsure if the Pills' design fits, but I think this has a solid chance and looks close enough.

src/components/PublicGroupList.vue Outdated Show resolved Hide resolved
src/components/PublicGroupList.vue Show resolved Hide resolved
@chakflying
Copy link
Collaborator

Expiry should not be shown if monitor type is not HTTPS.

@louislam
Copy link
Owner

image

I might prefer to put in front of the tag list. Like this.

@tarun7singh
Copy link
Contributor Author

I have incorporated tags design as suggested by @louislam and added check for monitor type (@chakflying ).
Screenshot 2023-07-17 at 1 53 47 AM

@tarun7singh
Copy link
Contributor Author

This is ready to be merged @CommanderStorm & @louislam

@chakflying chakflying mentioned this pull request Jul 20, 2023
Closed
1 task
@CommanderStorm
Copy link
Collaborator

Could you add Resolves #3456 to the description?

@maurerle
Copy link

maurerle commented Jul 20, 2023 •
edited
Loading

And also
resolves #2522 (well, it is not sortable, but sorting according to tags would be a quite different feature..?)

@tarun7singh
Copy link
Contributor Author

@maurerle
I can add #2522 to the description but should we create a new issue for the sortable part?

@CommanderStorm
Copy link
Collaborator

should we create a new issue for the sortable part?

I think this is included in #1610 => no need to create another issue

@louislam louislam merged commit 27ce472 into louislam:master Jul 31, 2023
@tarun7singh tarun7singh deleted the status-page-expiry branch July 31, 2023 14:07
@marvinruder marvinruder mentioned this pull request Aug 26, 2023
Closed
1 task
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@CommanderStorm CommanderStorm CommanderStorm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.23.0
5 participants
@tarun7singh @chakflying @louislam @CommanderStorm @maurerle