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

Inactive input-message adds white-space below a label #5130

Open
nwhittaker opened this issue Aug 10, 2022 · 19 comments
Open

Inactive input-message adds white-space below a label #5130

nwhittaker opened this issue Aug 10, 2022 · 19 comments
Labels
1 - assigned Issues that are assigned to a sprint and a team member. ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. has workaround Issues have a workaround available in the meantime. needs milestone Planning workflow - pending milestone assignment, has priority and/or estimate.

Comments

@nwhittaker
Copy link
Contributor

nwhittaker commented Aug 10, 2022

Actual Behavior

Given a <calcite-label> that contains a <calcite-input> and an inactive <calcite-input-message>, its bottom white-space is greater than a <calcite-label> that only contains a <calcite-input>.

Expected Behavior

Given a <calcite-label> that contains a <calcite-input> and an inactive <calcite-input-message>, its bottom white-space is the same as a <calcite-label> that only contains a <calcite-input>.

Reproduction Sample

https://codepen.io/nwhittaker-esri/pen/QWmBoEe
Updated codepen - https://codepen.io/mac_and_cheese/pen/BaxMBmm

Reproduction Steps

  1. Visit the code sample
  2. Note how the boxes around each <calcite-label> are different heights

The height of the 2nd box should match the height of the 1st box.

Reproduction Version

beta.86

Relevant Info

This inconsistency adds difficulty to maintaining a vertical rhythm within a form that includes only some inputs with an assoc. input message.

A quick fix could be to give <calcite-input-message> a display: none style when its active attribute is absent.

Regression?

No response

@nwhittaker nwhittaker added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Aug 10, 2022
@jcfranco jcfranco added the has workaround Issues have a workaround available in the meantime. label Oct 12, 2022
@geospatialem geospatialem removed the needs triage Planning workflow - pending design/dev review. label Oct 12, 2022
@alisonailea alisonailea self-assigned this Dec 12, 2022
@alisonailea alisonailea added 2 - in development Issues that are actively being worked on. and removed 0 - new New issues that need assignment. labels Dec 12, 2022
@alisonailea
Copy link
Contributor

The demo of the buggy behavior of Calcite Input Message is out of date. There has been some major changes to Calcite Input Message between .557 and .675. @benelan please update the demo or make a comment for Nathan describing how the component works now.

@alisonailea alisonailea removed their assignment Dec 14, 2022
@alisonailea alisonailea added incomplete issue report New issues missing important information, and unless provided, they will be closed after 5 days. and removed 2 - in development Issues that are actively being worked on. labels Dec 14, 2022
@benelan
Copy link
Member

benelan commented Dec 14, 2022

I'm still able to reproduce on next, none of the changes effected the repro sample:
https://codepen.io/benesri/pen/oNyrqLY

Edit:
as mentioned in the changelog

input-message: removed deprecated properties

  • Removed active property, use the global hidden attribute instead.
  • Removed type property, "floating" is no longer supported.

@benelan benelan added 0 - new New issues that need assignment. and removed incomplete issue report New issues missing important information, and unless provided, they will be closed after 5 days. labels Dec 14, 2022
@alisonailea alisonailea self-assigned this Dec 14, 2022
@alisonailea alisonailea added 2 - in development Issues that are actively being worked on. and removed 0 - new New issues that need assignment. labels Dec 14, 2022
@driskull
Copy link
Member

@eriklharper @macandcheese @benelan wasn't it the plan for this component to go away and be rolled into input?

If so, should we deprecate it?

@macandcheese
Copy link
Contributor

I know it has been mentioned before, although I can't find an issue for it. I think it might have to be rolled into label if possible, since it is applicable to a lot of form components, not just input.

It might be good to re-evaluate this as part of the larger form / validation story. I'd imagine that will be a larger epic for another major release, so not sure if we are quite ready to deprecate it?

@benelan
Copy link
Member

benelan commented Dec 15, 2022

I honestly thought we were going into the opposite direction by splitting up input. If we roll input-message into input do we just delete input-text and input-number?

@macandcheese
Copy link
Contributor

macandcheese commented Dec 15, 2022

By rolled into, I just meant having a slot within label for the component to reside. Right now input-message as a name is a bit misleading, really it should be / can be used for any form related component messaging, IMO. form-message or label-message, etc. seems more general / fitting. But I don't know if we need to change this until the next large breaking release after 1.0.

@benelan
Copy link
Member

benelan commented Dec 15, 2022

Yeah I think that one should wait. Input needs a lot of work; it's all over the place right now. Besides the validation/input-message items you mentioned:

  • Assuming we want to keep splitting up Input into separate components, we need to
    • start removing types from calcite-input. Right now you can do calcite-input-number and calcite-input type="number". We need to pick one. †
    • decide if we are creating a component for every type, which seems excessive to me. If we are keeping some types in the calcite-input component, we need to define which ones stay (and why)
    • create interfaces/utils to cut down on the duplicate code
  • Autofill/autocomplete doesn't work well and we will likely need to "unshadow" Input(s). At least until there is better browser support for forms with custom elements
  • The lifecycle methods need a refactor

I think it would be better to get all/most of those changes into a single major release if possible.


† It may make sense to remove input-text and input-number before v1 and then add them back when we have time to split the rest of the components, if that's the route we choose. I'll bring that up with Franco when he gets back

@macandcheese
Copy link
Contributor

macandcheese commented Dec 15, 2022

I think that’s a fair summary, although I do think this particular issue scope is much smaller - just fixing the white space with our current component - #6037

Maybe an issue with the above thoughts would be good?

@benelan
Copy link
Member

benelan commented Dec 15, 2022

Yeah I agree that we should fix this white space issue now. But deprecating/moving the input-message component should wait until we sort out our Input plans.

@driskull
Copy link
Member

Adding it as part of label might make the most sense. Or at least a slot of label.

If we do keep it then we should consider renaming it. It's really a form or label message

@benelan
Copy link
Member

benelan commented Dec 15, 2022

Yeah I like the idea of making it part of label. Input isn't the only form component that can benefit from validation messages

@driskull
Copy link
Member

Agreed. It should not be tied to just input

@alisonailea
Copy link
Contributor

@driskull @benelan the issue with the whitespace "bug" here is two fold. The components involved here use margin which is pushing around content they don't personally have control over (child controlling the layout of it's parent which is a design system anti-pattern) and that the label itself is using flex-gap to simultaneously layout it's children which adds a gap between input and input-message unless input-message's styles are set to "display: none" which can only be achieved by either setting the attribute on the Host or by a user setting the "hidden" attribute themselves. A full fix will require a refactor of label as well as input and input-message. OR documentation instructing users to simply add "hidden" attribute to empty input-message which seems like a better solution at this time.

@driskull
Copy link
Member

Agreed. Lets hold off on this one until Franco is back. Maybe we can deprecate this component and build it into label after 1.0.

@alisonailea
Copy link
Contributor

where should the documentation go?

@benelan
Copy link
Member

benelan commented Dec 15, 2022

Yeah agreed. I'd put it on the component's documentation page (source) under a new Overview/Usage section. Copying the format from other component doc pages, e.g. alert.

@geospatialem do you have bandwidth to help add the doc?

@geospatialem
Copy link
Member

@geospatialem do you have bandwidth to help add the doc?

Definitely, can add it to the doc page today, assuming we're adding a deprecation to the input-message component?

@alisonailea
Copy link
Contributor

Definitely, can add it to the doc page today, assuming we're adding a deprecation to the input-message component?

No deprecation yet. But we need to alert users that they must add a "hidden" attribute to an empty input-message or there will be weird extra spacing.

@geospatialem
Copy link
Member

Added to the doc site, which will go live next Wednesday. Updating to the Stalled milestone and Franco's list for consideration for his 1.0 and/or post-1.0 thoughts.

@benelan benelan added ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. and removed ArcGIS Field Apps labels Jan 30, 2023
@alisonailea alisonailea modified the milestone: Stalled Feb 28, 2023
@alisonailea alisonailea added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 2 - in development Issues that are actively being worked on. labels Dec 9, 2023
@brittneytewks brittneytewks added the needs milestone Planning workflow - pending milestone assignment, has priority and/or estimate. label Feb 20, 2025
@brittneytewks brittneytewks removed this from the Stalled milestone Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - assigned Issues that are assigned to a sprint and a team member. ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. has workaround Issues have a workaround available in the meantime. needs milestone Planning workflow - pending milestone assignment, has priority and/or estimate.
Projects
None yet
Development

No branches or pull requests

8 participants