Skip to content
This repository has been archived by the owner on Aug 30, 2019. It is now read-only.

Add header to SlidingPanel component #137

Merged
merged 3 commits into from
Aug 8, 2017
Merged

Add header to SlidingPanel component #137

merged 3 commits into from
Aug 8, 2017

Conversation

yurist38
Copy link
Contributor

@yurist38 yurist38 commented Aug 8, 2017

WHAT DOES THIS PR DO:

  • Added slidingPanelHeader component (for now internally, inside slidingPanel).
  • Added optional prop title to slidingPanel component. If it's defined we do show header.
  • slidingPanel component is changed to support multiple close buttons.

SCREENSHOT:

slidingpanelpreview

WHERE SHOULD THE REVIEWER START:

  • Diffs

UNIT TESTS:

  • Added and adjusted to achieve 100% coverage

@@ -1,4 +1,6 @@
import React, { Component, PropTypes } from 'react';
import React, { Component } from 'react';
import PropTypes from 'prop-types';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


const SlidingPanelHeader = ({ title }) => {
if (!title) {
return <noscript />;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaik could be null

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was null in my first implementation. But then I stuck with testing, it causes this error. Seems like unless we migrated to React 15 we still need to use noscript. By the way, I've searched other components and found many places where we also use noscript. Once we migrated I think could be nice to get rid of them.

Copy link
Contributor

@mAiNiNfEcTiOn mAiNiNfEcTiOn Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forget every time:D When we'll migrate?

@@ -8,19 +8,20 @@ describe('SlidingPanel', () => {
describe('#render()', () => {
it('render with default props and mods provided', () => {
const renderTree = mount(
<SlidingPanel mods={['test']}>Test</SlidingPanel>
<SlidingPanel mods={['test']} title="Test Title">Test</SlidingPanel>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not default props, please add new test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay. Got it. Will do now

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling d2ae93e on sliding-panel-header into cc53a02 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 31a51f4 on sliding-panel-header into cc53a02 on master.

@yurist38 yurist38 self-assigned this Aug 8, 2017
@asci asci merged commit ba490fc into master Aug 8, 2017
@yurist38 yurist38 deleted the sliding-panel-header branch August 16, 2017 09:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants