Skip to content

31894 and 32573 Updated par value validation and display#843

Merged
severinbeauvais merged 5 commits intobcgov:mainfrom
severinbeauvais:31894
Mar 3, 2026
Merged

31894 and 32573 Updated par value validation and display#843
severinbeauvais merged 5 commits intobcgov:mainfrom
severinbeauvais:31894

Conversation

@severinbeauvais
Copy link
Collaborator

@severinbeauvais severinbeauvais commented Mar 2, 2026

Issue #: bcgov/entity#31894 and bcgov/entity#32573

Description of changes:

Commit 1:

  • app version = 5.16.32
  • used default header (same as edit UI)
  • updated share classes styling to match Edit UI
  • created function to format par values
  • updated par value rules
  • created function to count significant digits
  • added/updated unit tests

Commit 2:

  • implemented Copilot suggestions

Commit 3:

  • added "ul" tags around list items
  • moved FormatCurrency to own util file
  • refactored FormatCurrency
  • added unit tests for FormatCurrency

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).

- used default header (same as edit UI)
- updated share classes styling to match Edit UI
- created function to format par values
- updated par value rules
- created function to count significant digits
- added/updated unit tests
@severinbeauvais severinbeauvais self-assigned this Mar 2, 2026
Copilot AI review requested due to automatic review settings March 2, 2026 19:26
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates share structure par value validation and display to align with the Edit UI, including introducing a significant-digits-based validation rule and formatting/par-value display changes.

Changes:

  • Add SignificantDigits utility + unit tests and wire it into par value validation.
  • Update Share Structure validation messaging/tests for the new par value rules.
  • Update List Share Class table header/styling and add par value formatting for display.

Reviewed changes

Copilot reviewed 7 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/unit/SignificantDigits.spec.ts Adds unit coverage for the new significant digits utility.
tests/unit/ShareStructure.spec.ts Updates par value validation tests to match new significant-digit rule/message.
src/utils/index.ts Re-exports SignificantDigits from the utils barrel.
src/utils/SignificantDigits.ts Introduces significant digits counting helper.
src/components/common/ShareStructure.vue Replaces decimal-place rules with significant-digits validation for par value.
src/components/common/ListShareClass.vue Updates share class/series table header/styling and formats displayed par values.
package.json Bumps app version to 5.16.32.
package-lock.json Updates lockfile version metadata (and adds a license field entry).
.gitignore Ignores Copilot instruction/agent files under .github.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +154 to +156
<li>
<span class="ml-n1">{{ seriesItem.name }}</span>
</li>
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

The template renders an <li> directly inside a <td> without a surrounding <ul>/<ol>, which is invalid HTML and can cause inconsistent styling / screen-reader output. Replace the <li> with a non-list element (eg, <span>/<div>) or wrap the series rows in a proper list container if list semantics are intended.

Suggested change
<li>
<span class="ml-n1">{{ seriesItem.name }}</span>
</li>
<span class="ml-n1">{{ seriesItem.name }}</span>

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I copied this from Edit UI. It displays just the bullet properly in all modern browsers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Invalid HTML though...
image

But if it works it works and worth keeping consistent between UIs, on one hand

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I'll just fix in both UIs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

</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.

I deleted this code in favour of using the default header -- same as in Edit UI.

{{ row.item.name }}
</td>
<td class="share-series-value">
<td class="share-series-value text-right">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Similar alignment as in Edit UI:

Image

<span>{{ seriesItem.name }}</span>
<li>
<span class="ml-n1">{{ seriesItem.name }}</span>
</li>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Similar indent and bullet as in Edit UI:

Image

}

td:not(:first-child){
color: $gray6;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gray6 is not a colour we should be using.

I updated some colours here to match Edit UI:

Image


# Copilot custom instructions
/**/.github/copilot-instructions.md
/**/.github/agents/
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In case you configure some AI tools in your VS Code 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@severinbeauvais severinbeauvais changed the title 31894 Updated par value validation and display 31894 and 32573 Updated par value validation and display Mar 2, 2026
Copy link
Collaborator

@meawong meawong left a comment

Choose a reason for hiding this comment

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

LGTM!

/** Returns a formatted par value. */
formatParValue (item: any): string {
/** Returns a number formatted with a minimum of two decimal places. */
function formatWithMinTwoDecimals (value: number): string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any need on toLocaleString for the par value formatted? The max shares above uses it in the template so I think would show 1,000,000 and then par value would show 1000000 if I'm understanding.
Not sure if realistic use case...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding grouping (commas) wasn't a requirement but I didn't like some of my examples anyways so I'm rewriting this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've completed my refactoring.

Severin Beauvais added 3 commits March 3, 2026 09:53
- moved FormatCurrency to own util file
- refactored FormatCurrency
- added unit tests for FormatCurrency
@severinbeauvais
Copy link
Collaborator Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

bcregistry-sre commented Mar 3, 2026

@bcgov bcgov deleted a comment from bcregistry-sre Mar 3, 2026
vuetify,
propsData: { shareClasses }
})
await flushPromises()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was an attempt to fix the broken unit tests in this test suite.

The root cause was actually that our CI environment doesn't support maximumFractionDigits: 100.

The current change is always a good idea and is consistent with other unit tests so I'm leaving it.

Copy link
Collaborator

@loneil loneil 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, would want QA to put it through it's paces when deployed trying all sorts of options, and saving/completing the filing too, so that the DB field (not just he filing json field) for par value is verified for these conditions

@severinbeauvais
Copy link
Collaborator Author

severinbeauvais commented Mar 3, 2026

Looks good, would want QA to put it through it's paces when deployed trying all sorts of options, and saving/completing the filing too, so that the DB field (not just he filing json field) for par value is verified for these conditions

I filed my test IA. The new business is: https://dev.business-dashboard.bcregistry.gov.bc.ca/BC0888490

Also, the tickets have the UX Assurance label on them already :)

@severinbeauvais severinbeauvais merged commit 69269e9 into bcgov:main Mar 3, 2026
8 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