search

techvalidate / pano

MIT License
0 stars 0 forks source link

Centering Modals Change #55

Closed mmlindeboom closed 7 years ago

mmlindeboom commented 7 years ago

This PR contains breaking changes

This PR provides a css based solution for vertically and horizontally centering modals, in addition to more semantic naming for modals.

I felt names like .modal-container and .modal-wrapper were becoming too ambiguous. I suggest the following changes:

Example markup for a new modal would be:

.modal#example-modal
  .modal-dialog
    .modal-content
      .modal-header
        Foo
      .modal-body
        Bar
      .modal-footer
        =btn_to 'OK, '#'
  .modal-bg

Given these are breaking changes, this may need to be merged into a release branch separate from stable to be coordinated for a release later. @jmckible @tjdo, let me know your thoughts on how much work this would be to implement in CX if we chose to adopt it.

Pano Review Process Thoughts

I'd like to use this PR as an example for future Pano PRs, in that we should be asking anyone who might be affected by the changes for a review, so it gives each team a chance to test/consider the changes and align them with their work stream. Please feel free to comment on this proposal / add thoughts you have on the process.

tjdo commented 7 years ago

In regards to thoughts for the process:

This can work for now.

It does require feedback from third parties using the library. Otherwise we risk breaking the work of these third parties.

It also requires a separate PR for each of our products using the library, so that each product can be QA'ed for the changes to the lib. This means that a GH issue will need to be opened so that our QA can be aware of the updates and test them accordingly. I'm not sure a PR alone is enough to make QA aware that the changes need to be tested.

To some degree, this process will be a negative for the library and the products using it, since most of the fixes and modifications will be made as overriding changes in the product code. These overriding changes are bound to be lost to history, unless we are extremely anal about recording and tracking these changes, and making sure they make it into a future release.

mmlindeboom commented 7 years ago

@tjdo This is true, and we'll have to create a release process around that in anticipation of any external users of Pano (issue here). As for our products, I agree this PR alone is not enough for QA. I think the process can start here, then it is incumbent on each team to roll out the changes as works best for their release schedule if the changes here are adopted into Pano.

Per @jmckible and @mkmiller request, I'll create a 2-2-stable branch and make that the base branch for this PR. At that point if everyone could approve their reviews so I confirm we have buy in, I'll merge it into 2-2-stable, and the teams can take it from there.