search

bcgov / bcrs-shared-components

BCRS Shared Components is a multi-package Lerna repository of shared Vue components, each published individually and that you can explore/ develop/ document/ test using Storybook.
https://bcgov.github.io/bcrs-shared-components/
Apache License 2.0
3 stars 30 forks source link

5475 more certify UI updates #37

Closed jordiwes closed 3 years ago

jordiwes commented 3 years ago

Issue #: /bcgov/entity#5475

Description of changes:

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

severinbeauvais commented 3 years ago

Looks good, one question though. The column size here on a regular laptop is shrinking to a 2 col but i think we want 3 everywhere, maybe @severinbeauvais would know about this as well?

Screen Shot 2021-03-19 at 12 04 05 PM
  1. We need to stop using v-flex as it's deprecated. It's easy to convert to v-row and v-col.
  2. We haven't had a good look at responsiveness (different screen widths), so kudos for doing it here. At some point the other components should be looked at. Wanna do that now?
cameron-eyds commented 3 years ago

Looks good, one question though. The column size here on a regular laptop is shrinking to a 2 col but i think we want 3 everywhere, maybe @severinbeauvais would know about this as well?

Screen Shot 2021-03-19 at 12 04 05 PM
  1. We need to stop using v-flex as it's deprecated. It's easy to convert to v-row and v-col.
  2. We haven't had a good look at responsiveness (different screen widths), so kudos for doing it here. At some point the other components should be looked at. Wanna do that now?

I know from experience going back and forth between Shared and the UI where the components are used can be a pain in the butt :) I would try and do it now, to save the hassle later!

I believe this would apply to both rows as well.

jordiwes commented 3 years ago

So I changed to 3 & 9 columns vs 2 & 10 to match what Severin had in the alteration date & time, but not sure if there is too much white space now? Might be fine:

image

severinbeauvais commented 3 years ago

So I changed to 3 & 9 columns vs 2 & 10 to match what Severin had in the alteration date & time, but not sure if there is too much white space now? Might be fine:

What does it look like in use (in app) ? Hopefully our UI designs will be consistent w.r.t. label and content spacing, ie. 3+9 for everything. Either way, have a look it and make sure it matches the comps.

Another thing you could do (but only if needed) is to pass in the first/second columns width as props (with default 3 & 9). This would allow this component to work in various apps (if they have different requirements).

jordiwes commented 3 years ago

I made the columns props, as I know corrections use 2 & 10

severinbeauvais commented 3 years ago

@jordiwes Please use the standard template for this PR's description block... and then I will merge this change.

PS Let me know if you need help with the rest of the shared components steps.

jordiwes commented 3 years ago

Done

severinbeauvais commented 3 years ago

Oh no. I just noticed you added some commits after my review, and they are incorrect (and I merged them).

It appears you did not follow the steps in the README.

You do not update the component versions. Lerna does that for you.

Try to reconcile what you did with the steps in the readme and continue from there. You may or may not have sufficient permissions to push tags or publish -- if not, let me know and I will do that for you.