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

Fix: form visibility #507

Merged
merged 4 commits into from Jul 29, 2021
Merged

Fix: form visibility #507

merged 4 commits into from Jul 29, 2021

Conversation

mturoci
Copy link
Collaborator

@mturoci mturoci commented Jan 18, 2021

Moved the visibility handling logic to parent form component instead of dealing with it at component level.

There was also a minor spacing issue - we applied marginTop: 0 to :not(:first-child) selector. The problem is, this selector looks at the DOM (hidden elements are still present in the DOM), not whether the element is visible or not. I managed to fix it via CSS.

Closes #484
Closes #817

@mturoci mturoci requested a review from lo5 as a code owner January 18, 2021 15:46
@lo5
Copy link
Member

lo5 commented Jan 19, 2021

Any reason why this uses visibility: hidden instead of the previous display: none?
Also, we should find a non-js solution to spacing.

@mturoci
Copy link
Collaborator Author

mturoci commented Jan 19, 2021

Any reason why this uses visibility: hidden instead of the previous display: none?

Futurewise, display: none cannot be animated. This means that after resolving #229 (form components will be properly updated instead of recreated on props change), we can make toggled elements leave smoothly instead of snapping. Apart from that, there is not really much of a difference.

Also, we should find a non-js solution to spacing.

I tried to look for something better, but can try harder.

@mturoci
Copy link
Collaborator Author

mturoci commented Jan 19, 2021

Found a pure CSS approach for spacing. Added custom data attr data-visible to get info whether the element is currently visible or not. Then used > [data-visible="true"] ~ div' to select all direct form elements that are preceded by visible elements. This is a working solution for "first visual child and onwards.

Also added the needed functionality to horizontal (inline) part.

@mturoci mturoci force-pushed the fix/issue-484 branch 2 times, most recently from 40a9003 to eec3f1b Compare January 20, 2021 10:31
@mturoci
Copy link
Collaborator Author

mturoci commented Jan 20, 2021

  • Rebased.
  • Replaced visibility: hidden with display:none as discussed.
  • Added example demonstrating the feature.
Screen.Recording.2021-01-20.at.11.36.21.AM.mov

Needs discussion: @lo5 this approach will not work for cases when visible is defined on elements in ui.buttons as it creates its own wrapper at component level. A few ways to resolve this:

  • Shouldn't ui.buttons be deprecated in favor of ui.inline or you left it there just for the sake of not introducing a breaking change?
  • Special case it within button component.

@mturoci
Copy link
Collaborator Author

mturoci commented Mar 4, 2021

Rebased against current master.

Copy link
Member

@lo5 lo5 left a comment

Choose a reason for hiding this comment

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

This PR causes a breaking change: the visible attribute has been removed from the API.

@mturoci
Copy link
Collaborator Author

mturoci commented Mar 5, 2021

TBH I don't remember the reasoning why visible was removed from each component. Adding it back to API should cause no harm as it is used anyway.

@mturoci mturoci force-pushed the fix/issue-484 branch 2 times, most recently from aea0c29 to 6aaecd6 Compare March 5, 2021 08:55
@mturoci
Copy link
Collaborator Author

mturoci commented Mar 5, 2021

  • rebased
  • reverted back visible prop per component to avoid breaking change
  • special cased the button component to allow using visible prop even inside ui.buttons component.

@mturoci
Copy link
Collaborator Author

mturoci commented May 13, 2021

Rebased.

@mtanco
Copy link
Contributor

mtanco commented Jun 1, 2021

@mturoci seeing similar behavior for Toggles, would that be handled automatically in this PR too or should I open a separate issue?

image

Wave 0.16.0

from h2o_wave import site, ui

page = site['/']

page['toggle'] = ui.form_card(
    box='4 4 4 4',
    items=[
        ui.toggle(name='hidden', label='You can\'t see me', visible=False),
        ui.toggle(name='shown', label='You can see me', visible=True),
    ]
)
page.save()

@mturoci
Copy link
Collaborator Author

mturoci commented Jun 2, 2021

@mtanco This PR fixes visibility for all the form items at once so no need to open a new issue.

@lo5
Copy link
Member

lo5 commented Jul 6, 2021

@mturoci - what would be the minimal fix for this bug (without changes to the generator, or introducing changes to launch.json, precommit.js, etc.)?

@mturoci
Copy link
Collaborator Author

mturoci commented Jul 6, 2021

The minimal changes are moving visibility handling logic from component scope to form scope. The generator is updated so that every form component has a consistent visible prop, but we can go with hardcoding it as it was before ofc.

@mturoci mturoci force-pushed the fix/issue-484 branch 2 times, most recently from 444edef to 1ba9103 Compare July 7, 2021 14:22
@mturoci
Copy link
Collaborator Author

mturoci commented Jul 7, 2021

@lo5 rebased against current master, removed all the extra stuff that is not necessarily part of the fix, removed the generator tweaks and settled back to specifying visible per each component.

@psinger
Copy link

psinger commented Jul 26, 2021

@mturoci What is the status of this issue? Will it be fixed soon? Thanks!

@mturoci mturoci merged commit 8e5498b into master Jul 29, 2021
@mturoci mturoci deleted the fix/issue-484 branch July 29, 2021 05:54
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.

Message bar visibility doesn't hide component Visible prop on textbox not working
4 participants