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

GN-4483: Remove styling for #ember-basic-dropdown-wormhole #958

Merged
merged 2 commits into from
Sep 6, 2023

Conversation

dkozickis
Copy link
Contributor

@dkozickis dkozickis commented Sep 5, 2023

Overview

Remove styling for #ember-basic-dropdown-wormhole as it may break the styling for applications embedding the editor.

Connected issues and PRs

https://binnenland.atlassian.net/browse/GN-4483

Setup

  1. Checkout GN-4483: Remove styling for #ember-basic-dropdown-wormhole frontend-embeddable-notule-editor#128
  2. Checkout this PR
  3. In the embeddable repo do a npm link ../ember-rdfa-editor/ (depends on where you have the editor checked out)
  4. In the embeddable repo npm run start

How to test/reproduce

  • Observe that the dropdown for the Inline variable renders in the correct place when fix from the editor is npm linked
    CleanShot 2023-09-05 at 11 56 31@2x

  • Observe that the dropdown is rendered in the wrong place when the fix from the editor is not npm linked

CleanShot 2023-09-05 at 11 59 50@2x

Challenges/uncertainties

Checks PR readiness

  • UI: works on smaller screen sizes
  • UI: feedback for any loading/error states
  • Check cancel/go-back flows
  • Check database state correct when deleting/updating (especially regarding relationships)
  • changelog
  • npm lint
  • no new deprecations

Copy link
Contributor

@x-m-el x-m-el left a comment

Choose a reason for hiding this comment

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

see lblod/frontend-embeddable-notule-editor#128 (review) for a detailed review.

The part referring to code here is a question if z-index: 9999 might be doing something good (bugfix) that we don't know about. It might be an idea to leave that code in.

But I personally lean to just removing it, as I can't reproduce any bug that would need it 🤷

@abeforgit abeforgit enabled auto-merge September 6, 2023 13:44
@abeforgit abeforgit merged commit b393371 into master Sep 6, 2023
@abeforgit abeforgit deleted the GN-4483 branch September 6, 2023 13:49
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.

3 participants