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

22579 - Add FED Column & Tooltips (continuation table) #2978

Merged

Conversation

ArwenQin
Copy link
Contributor

@ArwenQin ArwenQin commented Aug 21, 2024

Issue #:
bcgov/entity#22579

Description of changes:

  • Added Future Effective Date column to table
  • Added red/orange tooltips to future effective date
  • Added red/orange tooltips to NR number when it has expired /<14 days
  • Added action for NR redirect to Namex UI
  • Added hover text showing what statuses are selected
  • For date pickers: show the date range truncated and the full text on hover
  • Added close icon to all search fields
  • Fixed the bug: calendar icon can't open the date picker

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the sbc-auth license (Apache 2.0).

@ArwenQin
Copy link
Contributor Author

ArwenQin commented Aug 21, 2024

Updated the Table name, FED column and actions icon styles:
image

@severinbeauvais
Copy link
Collaborator

I think you should make the Status column wider so it fits without horizontal scrolling. You can make some other columns narrower, such as the dates and the NR number.

@severinbeauvais
Copy link
Collaborator

Ask Janis if the Name Examination should open in a new page (tab).

Copy link
Collaborator

@seeker25 seeker25 left a comment

Choose a reason for hiding this comment

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

LGTM, need entities review though

@ArwenQin
Copy link
Contributor Author

I think you should make the Status column wider so it fits without horizontal scrolling. You can make some other columns narrower, such as the dates and the NR number.

Updated, made the NR number, date and identifier column narrower.
Updated the redirect to NameX, will open in a new tab.
image

@ArwenQin

This comment was marked as resolved.

@bcgov bcgov deleted a comment from bcregistry-sre Aug 22, 2024
@bcgov bcgov deleted a comment from bcregistry-sre Aug 22, 2024
@severinbeauvais
Copy link
Collaborator

Updated, made the NR number, date and identifier column narrower. Updated the redirect to NameX, will open in a new tab.

Looks great!

Since there's an 'X' to clear the dates and Status, what do you think of adding an 'X' to clear the NR Number, Identifying Number and Completing Party?

image

@severinbeauvais
Copy link
Collaborator

PS You need to "pull up" the icons in the header by 1 or 2 px.

image

image

Also, can you check what the dropdown menu is attached to? It obscures its action button so I have to click away from it to close it.

image

@severinbeauvais
Copy link
Collaborator

Do you think we should be able to open the NR when the application is approved or rejected?

image

image

I'm thinking "yes" but check with Janis and/or Mihai.

@bcgov bcgov deleted a comment from bcregistry-sre Aug 23, 2024
@severinbeauvais
Copy link
Collaborator

image

This looks very good. Ask Janis if we should show hours instead of days if it's less than, say, 1 day.

NRs always expire at 11:59 pm on their last day, so we probably don't need to show hours for them. What does the tooltip say if the NR expires today?

Sample NR info:
image

@severinbeauvais
Copy link
Collaborator

Test results

With NR expiring in 6 days (2024-08-29):

image

With NR expiring today (2024-08-23):

image

image

Do you think this is OK, or should it show "expiring today"? And also, why isn't the icon red?

@ArwenQin
Copy link
Contributor Author

Test results

With NR expiring in 6 days (2024-08-29):

image

With NR expiring today (2024-08-23):

image

image

Do you think this is OK, or should it show "expiring today"? And also, why isn't the icon red?

Yes, if the NR expires in less than 24 hour, it will say "expires in 1 day". I think it's ok. Should I check with Janis?
I will check with Janis for the FED less than 24 hours.
The icon is red when the NR already expires (Figma below):
image

@severinbeauvais
Copy link
Collaborator

Yes, if the NR expires in less than 24 hour, it will say "expires in 1 day". I think it's ok. Should I check with Janis? I will check with Janis for the FED less than 24 hours. The icon is red when the NR already expires

Yes, check with Janis. As a possible solution, how about if the dates have their own tooltip like this?

image

Yes, you are correct about the red NR icon 👍

@ArwenQin
Copy link
Contributor Author

Yes, if the NR expires in less than 24 hour, it will say "expires in 1 day". I think it's ok. Should I check with Janis? I will check with Janis for the FED less than 24 hours. The icon is red when the NR already expires

Yes, check with Janis. As a possible solution, how about if the dates have their own tooltip like this?

image

Yes, you are correct about the red NR icon 👍

Sure, checking with Janis and cc you there.

@ArwenQin
Copy link
Contributor Author

Updated:

  • All statuses can open NR
    image

  • Updated the FED tooltip to show the full date and time
    image

  • Updated the NR date tooltip expiring today
    image

@@ -68,7 +163,12 @@
v-for="(header, i) in headers"
:key="getIndexedTag('find-header-row', i)"
:scope="getIndexedTag('find-header-col', i)"
class="font-weight-bold"
:class="{
'font-weight-bold': true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to change this but FYI: since this class is always set, you can use the class prop on this element. (Ie, you can use both class and :class.)

@bcgov bcgov deleted a comment from bcregistry-sre Aug 28, 2024
@bcgov bcgov deleted a comment from bcregistry-sre Aug 28, 2024
@severinbeauvais
Copy link
Collaborator

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://bcregistry-account-dev--pr-2978-f8g8fzgr.web.app

@@ -58,7 +58,7 @@
"@types/vuelidate": "^0.7.13",
"@typescript-eslint/eslint-plugin": "^6.4.0",
"@typescript-eslint/parser": "^6.4.0",
"@volar-plugins/vetur": "*",
"@volar-plugins/vetur": "latest",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please undo this change.

Also, please update the app version again.

Then, go ahead and merge this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Sev! I just undo this and updated the app version again.
Could you please merge it?
Thank you.

Copy link

sonarcloud bot commented Aug 28, 2024

@severinbeauvais severinbeauvais merged commit 9b533ef into bcgov:main Aug 28, 2024
5 of 6 checks passed
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