-
Notifications
You must be signed in to change notification settings - Fork 34
Contributions/solittlework/1813 datepicker width #2712
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
Contributions/solittlework/1813 datepicker width #2712
Conversation
|
This change only seems to work when using |
|
@chrisolsen @vanessatran-ddi Issue with percentage width has been fixed. And tests have been created for testing the width property. |
| type="number" | ||
| on:_change={onInputChange} | ||
| width="4ch" | ||
| width="3ch" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to change the width here? I see the width sets to "4ch" because of this issue #2703, https://github.com/GovAlta/ui-components/pull/2706/files
| type="number" | ||
| on:_change={onInputChange} | ||
| width="6ch" | ||
| width="5ch" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, looks like "6ch" is a correct value, not "5ch"
|
I just have a concern about the merged issue comments above. The rest I have tested and it works well with ch, % and px, both react and angular. Since it has changes on popover, I also made a test on dropdown, app header menu and it looks good. |
|
@ArakTaiRoth you have conflicts |
|
@chrisolsen All conflicts have been fixed |
| display: block; | ||
| width: 100%; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ArakTaiRoth Why are these changes being made? If I remove them I see no change to datepicker, but with them the target within a normal popover takes up 100% of the parent

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrisolsen Perhaps we should talk about this. This is what I see for the Datepicker with that change:

And this is what I see if I change it back to what it was (display: flex):

That all being said, I do see the issue with popover-target becoming 100% width, so that needs to be fixed.
967e661 to
db3e504
Compare
| font-family: var(--goa-font-family-sans); | ||
| font: var(--goa-typography-body-m); | ||
| display: inline; | ||
| display: block; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Closing, as this PR got messed up by previous changes. A new PR for this story is incoming. |


Before (the change)
Steps needed to test