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

22846 - show resolution dates in amalgamations #745

Merged
merged 6 commits into from
Oct 11, 2024

Conversation

ketaki-deodhar
Copy link
Collaborator

@ketaki-deodhar ketaki-deodhar commented Oct 10, 2024

Issue #: /bcgov/entity#22846

Description of changes:

  • fetch resolutions
  • show resolution dates on summary page
  • save resolution dates in filing
  • WIP - add more unit tests

Short form amalgamation (Horizontal):

image

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

@ketaki-deodhar ketaki-deodhar self-assigned this Oct 10, 2024
@ketaki-deodhar
Copy link
Collaborator Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

bcregistry-sre commented Oct 10, 2024

@ketaki-deodhar ketaki-deodhar marked this pull request as ready for review October 10, 2024 17:30
Copy link
Collaborator

@ArwenQin ArwenQin left a comment

Choose a reason for hiding this comment

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

Looks good!

@severinbeauvais
Copy link
Collaborator

image

I think the font size should be 14px, same as the other content in the table.

Also, the label colour should be #212529 and the value colour should be #495057.

You can confirm with Jacqueline if you want.

@severinbeauvais
Copy link
Collaborator

severinbeauvais commented Oct 10, 2024

While you're working on this component, Jacqueline would like the Share Structure table headers to have the same format as the People and Roles table headers (#212529, bold, 14px). Can you make this change or should that be another ticket? Update: done, thanks!

@ketaki-deodhar
Copy link
Collaborator Author

ketaki-deodhar commented Oct 10, 2024

While you're working on this component, Jacqueline would like the Share Structure table headers to have the same format as the People and Roles table headers (#212529, bold, 14px). Can you make this change or should that be another ticket?

No need for another ticket. I will do it here

@@ -39,7 +39,23 @@
disable-pagination
disable-sort
hide-default-footer
hide-default-header
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

used hide-default-header to apply custom style.

@ketaki-deodhar
Copy link
Collaborator Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://business-create-dev--pr-745-9wwaez00.web.app

</th>
</tr>
</thead>
</template>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As this is a common component, this is how it looks when just a list:
Before/DEV:
image

After:
image

When in edit mode:
Before/DEV:
image

After:
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome! Looks great.

@ketaki-deodhar
Copy link
Collaborator Author

image

I think the font size should be 14px, same as the other content in the table.

Also, the label colour should be #212529 and the value colour should be #495057.

You can confirm with Jacqueline if you want.

Also, Jacqueline to take a look at this

</td>
<td class="share-series-value">
{{ row.item.hasRightsOrRestrictions ? 'Yes' : 'No' }}
</td>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Jacqueline took a look at this and suggested to change the color for values

@jacqueline-williams-549

image

I think the font size should be 14px, same as the other content in the table.
Also, the label colour should be #212529 and the value colour should be #495057.
You can confirm with Jacqueline if you want.

Also, Jacqueline to take a look at this

Reviewed with Ketaki and getting back to her about suggestion about language

Copy link
Collaborator

@severinbeauvais severinbeauvais left a comment

Choose a reason for hiding this comment

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

LGTM. Remember to update the app version.

@ketaki-deodhar ketaki-deodhar merged commit 6be0690 into bcgov:main Oct 11, 2024
4 of 5 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.

5 participants