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

JENKINS-39371 Dialog Component #113

Merged
merged 1 commit into from Nov 13, 2016

Conversation

Projects
None yet
5 participants
@sophistifunk
Copy link
Collaborator

commented Nov 9, 2016

Description

Submitter checklist

  • [X ] Link to JIRA ticket in description, if appropriate.
  • [X ] Change is code complete and matches issue description
  • Appropriate unit or acceptance tests or explanation to why this change has no tests

Reviewer checklist

  • Run the changes and verified the change matches the issue description
  • Reviewed the code
  • Verified that the appropriate tests have been written or valid explanation given

@reviewbybees

Squashed commit of the following:

josh/JENKINS-39371-dialog-2 * Fix some remaining TODOs and an issue with proptypes
josh/JENKINS-39371-dialog-2 * Fix some flow / lint issues
josh/JENKINS-39371-dialog-2 * Add wrapper component to do it all easily, add some styles for input / error etc
josh/JENKINS-39371-dialog-2 * Add header component
josh/JENKINS-39371-dialog-2 * Fix a small flow issue
josh/JENKINS-39371-dialog-2 * Wire it up
josh/JENKINS-39371-dialog-2 * Initial merge of Dialog from old branch (won't run yet)
JENKINS-39371 Dialog Component
Squashed commit of the following:

    josh/JENKINS-39371-dialog-2 * Fix some remaining TODOs and an issue with proptypes
    josh/JENKINS-39371-dialog-2 * Fix some flow / lint issues
    josh/JENKINS-39371-dialog-2 * Add wrapper component to do it all easily, add some styles for input / error etc
    josh/JENKINS-39371-dialog-2 * Add header component
    josh/JENKINS-39371-dialog-2 * Fix a small flow issue
    josh/JENKINS-39371-dialog-2 * Wire it up
    josh/JENKINS-39371-dialog-2 * Initial merge of Dialog from old branch (won't run yet)
@reviewbybees

This comment has been minimized.

Copy link
Collaborator

commented Nov 9, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@i386

This comment has been minimized.

Copy link
Collaborator

commented Nov 9, 2016

FYI @kzantow this is the dialog for your editor step listing stories.

@i386

This comment has been minimized.

Copy link
Collaborator

commented Nov 9, 2016

@cliffmeyers would be good to get your eyes on this

@@ -69,12 +67,54 @@
align-items: center;
width: 100%;
height: 80px;
min-height: 80px;
flex-shrink: 0;

This comment has been minimized.

Copy link
@kzantow

kzantow Nov 10, 2016

Collaborator

can we use flex for this? I thought we had to change away from flex to get IE working properly

This comment has been minimized.

Copy link
@sophistifunk

sophistifunk Nov 10, 2016

Author Collaborator

This is news to me. If we see bugs on whatever is the minimum version of IE we support I guess we can address it there, but I'd go with an ie-specific stylesheet rather than dropping flex-box, as it's miles better and it's everywhere in the editor.

This comment has been minimized.

Copy link
@cliffmeyers

cliffmeyers Nov 10, 2016

Collaborator

I think the problems with flexbox were limited to how the top-level page template in blueocean-web was laid out, causing IE issues. Perhaps @tfennelly can clarify since I think he was the one who fixed them.

This comment has been minimized.

Copy link
@cliffmeyers

cliffmeyers Nov 11, 2016

Collaborator

I tested the stories on Win 8.1 + IE11 and Win 10 + Edge and saw no issues. Think this use of flexbox will be 👍

@kzantow
Copy link
Collaborator

left a comment

Seems good. Storybook works as advertised. Looks nice, btw.

@cliffmeyers

This comment has been minimized.

Copy link
Collaborator

commented Nov 10, 2016

Been bogged down in a few other things today but this will be priority one tomorrow, if not before.

@cliffmeyers
Copy link
Collaborator

left a comment

This looks great; just one small comment that is a tiny nit / question. Love the use of composition with all this.

render() {
return (
<div className="Dialog-header">
<h3>{this.props.children}</h3>

This comment has been minimized.

Copy link
@cliffmeyers

cliffmeyers Nov 11, 2016

Collaborator

In the event someone wanted to use Dialog but provide custom header content (icons, a button, etc), should we do anything a bit fancier here? Perhaps check if children is a string and wrap in h3, if a ReactElement omit the h3?

I know they can always use BasicDialog and built up their own, just wasn't sure it was a use-case we wanted to bother supporting.

@sophistifunk sophistifunk merged commit 69a2432 into master Nov 13, 2016

1 check passed

continuous-integration/jenkins/branch This commit looks good
Details

@sophistifunk sophistifunk deleted the feature/JENKINS-39371-dialog branch Nov 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.