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

fix(switch): backward compatibility in case labelOff is not set #2816

Merged
merged 1 commit into from Sep 18, 2019

Conversation

@boaz0
Copy link
Member

boaz0 commented Sep 2, 2019

What:

Add a check if label is set but not labelOff - in that case display label on both labels.
if both label and labelOff are set display in the new format
Otherwise - display switch without labels.

Added tests to verify that if labelOff not given the label is shown.

fixes #2814

//cc @SpyTec

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 2, 2019

PatternFly-React preview: https://patternfly-react-pr-2816.surge.sh

@boaz0 boaz0 force-pushed the boaz0:closes_2814 branch 2 times, most recently from f17236c to 265756b Sep 2, 2019
@boaz0 boaz0 force-pushed the boaz0:closes_2814 branch 3 times, most recently from 7cb6285 to a650603 Sep 15, 2019
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Sep 16, 2019

fixed merge conflicts

@jschuler

This comment has been minimized.

Copy link
Collaborator

jschuler commented Sep 16, 2019

Hi @boaz0 , what i meant is that we don't need to code duplication, we can simply fix the conditionals.
Before:

        {label !== '' || labelOff !== '' ? (
          <React.Fragment>
            <span className={css(styles.switchToggle)} />
            <span
              className={css(styles.switchLabel, styles.modifiers.on)}
              id={isAriaLabelledBy ? `${this.id}-on` : null}
              aria-hidden="true"
            >
              {label}
            </span>
            <span
              className={css(styles.switchLabel, styles.modifiers.off)}
              id={isAriaLabelledBy ? `${this.id}-off` : null}
              aria-hidden="true"
            >
              {labelOff}
            </span>
          </React.Fragment>
        ) : (
          <span className={css(styles.switchToggle)}>
            <div className={css(styles.switchToggleIcon)} aria-hidden="true">
              <CheckIcon noVerticalAlign />
            </div>
          </span>
        )}

After:

        {label !== '' ? (
          <React.Fragment>
            <span className={css(styles.switchToggle)} />
            <span
              className={css(styles.switchLabel, styles.modifiers.on)}
              id={isAriaLabelledBy ? `${this.id}-on` : null}
              aria-hidden="true"
            >
              {label}
            </span>
            <span
              className={css(styles.switchLabel, styles.modifiers.off)}
              id={isAriaLabelledBy ? `${this.id}-off` : null}
              aria-hidden="true"
            >
              {labelOff || label}
            </span>
          </React.Fragment>
        ) : (
          <span className={css(styles.switchToggle)}>
            <div className={css(styles.switchToggleIcon)} aria-hidden="true">
              <CheckIcon noVerticalAlign />
            </div>
          </span>
        )}
@tlabaj tlabaj requested a review from redallen Sep 17, 2019
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Sep 17, 2019

@jschuler I got it.

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
@boaz0 boaz0 force-pushed the boaz0:closes_2814 branch from a650603 to c5f0327 Sep 17, 2019
@tlabaj
tlabaj approved these changes Sep 18, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit f2d0969 into patternfly:master Sep 18, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.