zakodium-oss / react-science

React components and tools to build scientific applications.
https://react-science.pages.dev
MIT License
3 stars 6 forks source link

modal delete button position issue #593

Closed hamed-musallam closed 11 months ago

hamed-musallam commented 11 months ago

It appears to be incorrect since you are applying the style to the button inside a span, which should not be the case. This will cause issues, especially with test tools like Playwright, which waits for the element to be visible, enabled, and stable.

https://github.com/zakodium-oss/react-science/blob/fa300f8046149a9a4bdf78a227a82c8ee9f80ad4/src/components/modal/Modal.tsx#L118

https://github.com/zakodium-oss/react-science/blob/fa300f8046149a9a4bdf78a227a82c8ee9f80ad4/src/components/button/Button.tsx#L25-L29

hamed-musallam commented 11 months ago

Could we add a custom role or class to make it easy to access the delete button using the CSS selector?

hamed-musallam commented 11 months ago

@stropitek

WDT ? Can we address and resolve it for the upcoming release, which will include fixes for this issue as well as those already resolved in https://github.com/zakodium-oss/react-science/pull/592

stropitek commented 11 months ago

The Modal component is going to be completely rewritten by https://github.com/zakodium-oss/react-science/issues/550. Can this wait?

hamed-musallam commented 11 months ago

this is a simple change we could wrap the delete button inside the modal with div and apply the CSS over it for now , it is a temporary solution until the rewritten is done


     <div  style={{ position: 'absolute', top: 4, right: 4 }}>
       <Button
                  onClick={onRequestClose}
                  intent="danger"
                  minimal
                  icon="cross"
                />
        </div>
hamed-musallam commented 11 months ago

Alternatively, I could trigger the button press even if the button is not fully ready from the Playwright perspective, this is something i need to check if you see that this change we can not do right now

stropitek commented 11 months ago

Why is having a span wrapping a button issue in general / with playwright?

There is a renderTarget prop in blueprintjs's Tooltip component which may help getting rid of the unnecessary span.

stropitek commented 11 months ago

There is a renderTarget prop in blueprintjs's Tooltip component which may help getting rid of the unnecessary span.

@hamed-musallam Can you try this and create a PR?

https://blueprintjs.com/docs/#core/components/tooltip

hamed-musallam commented 11 months ago

@stropitek

Could you review the pull request? The button is no longer wrapped with a span after we set the render target for the tooltip