search

scania-digital-design-system / tegel

Tegel Design System
https://tegel.scania.com
MIT License
15 stars 12 forks source link

fix(tds-modal): don't close when show prop is provided #648

Closed Lunkan89 closed 1 week ago

Lunkan89 commented 1 month ago

Describe pull-request

Describe what the pull-request is about

Issue Linking:

Choose one of the following options

How to test

Provide detailed steps for testing, including any necessary setup.

  1. Go to...
  2. Check in...
  3. Run ...

Checklist before submission

Suggested test steps

Screenshots

Include before/after screenshots for UI changes.

Additional context

Add any other context or feedback requests about the pull-request here.

aws-amplify-eu-north-1[bot] commented 1 month ago

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-648.d3fazya28914g3.amplifyapp.com

Lunkan89 commented 1 month ago

Using this issue to learn the work flow. I dont have access to Jira yet so cant reference connected jira issue. Assign myself to related issue: https://github.com/scania-digital-design-system/tegel/issues/614

Lunkan89 commented 1 month ago

@nathalielindqvist Maybe we should ignore the interaction with the reference element altogether if the show prop is provided, giving full control of show/hide to implementation? or am i miss understanding?

nathalielindqvist commented 1 month ago

@nathalielindqvist Maybe we should ignore the interaction with the reference element altogether if the show prop is provided, giving full control of show/hide to implementation? or am i miss understanding?

@Lunkan89 I think we have used the reference element to make it super clear how the component can be implemented in an actual application and not just rely on the Storybook controls. Since we have our demo pages in both Reach and Angular we could probably remove the reference element from our Storybook example. However, I still think the close button in the Modal itself should change the 'show' prop and the Modal should be hidden. Otherwise the Modals behavior might be misinterpreted, maybe?

Lunkan89 commented 1 month ago

@nathalielindqvist This maybe a non-issue and the answer to https://github.com/scania-digital-design-system/tegel/issues/614 should just be to use closeable = false if desired behavior is to not be able to close modal on show = true. This PR maybe obsolete?

timrombergjakobsson commented 1 month ago

@nathalielindqvist This maybe a non-issue and the answer to #614 should just be to use closeable = false if desired behavior is to not be able to close modal on show = true. This PR maybe obsolete?

Well the closeable prop controls whether the close button is displayed. However, it does not inherently prevent the modal from closing through other interactions, like clicking the overlay (unless the prevent prop is set to true).

sonarcloud[bot] commented 3 weeks ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud