-
Notifications
You must be signed in to change notification settings - Fork 55
feat(docs): adding 'behaviors' section to menu #119
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure this PR contains as few changes as possible. It seems that is contains a lot of changes not directly related to it.
{ key: 'editorials', content: 'Editorials' }, | ||
{ key: 'review', content: 'Reviews' }, | ||
{ key: 'events', content: 'Upcoming Events' }, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this debug changes
@@ -0,0 +1,35 @@ | |||
import * as React from 'react' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changed tests are not part of 'Accessibility documentation', these should be a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this actually is a separate PR (https://github.com/stardust-ui/react/pull/74/files) - once that gets merged, the change should disappear from this PR.
Codecov Report
@@ Coverage Diff @@
## master #119 +/- ##
=======================================
Coverage 68.02% 68.02%
=======================================
Files 101 101
Lines 1370 1370
Branches 260 269 +9
=======================================
Hits 932 932
Misses 436 436
Partials 2 2 Continue to review full report at Codecov.
|
if (!_.isEmpty(blockComments)) { | ||
const commentTokens = doctrine.parse(blockComments[0].raw, { unwrap: true }).tags | ||
const descriptionToken = commentTokens.find(token => token.title === 'description') | ||
description = descriptionToken.description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check descriptionToken
is defined. Now if there is a block comment without @description
build crashes.
@@ -78,7 +91,12 @@ task('build:docs:example-menu', () => | |||
|
|||
task( | |||
'build:docs:json', | |||
parallel('build:docs:docgen', 'build:docs:component-menu', 'build:docs:example-menu'), | |||
parallel( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Watch (yarn start
) does not reflect changes if Behavior description changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point :)
I was trying to implement that watch will rebuild either behaviors file, but was not able to find how to do it.
@miroslavstastny or @kuzhelov pleae if you could help me with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added watch for behaviours. Watch currently does not work on Windows (not caused by this PR), I will address that in #172 once this PR is merged.
@@ -55,6 +61,7 @@ task( | |||
// ---------------------------------------- | |||
|
|||
const componentsSrc = [`${config.paths.src()}/components/*/[A-Z]*.tsx`] | |||
const behaviorSrc = [`${config.paths.src()}/lib/accessibility/Behaviors/*/[A-Z]*.ts`] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about DefaultBehavior.ts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not real behavior which would need documentation, so it doesn't need be included
Also add |
} | ||
|
||
getNameFromFileName(fileName: string) { | ||
const divided = _.startCase(fileName.replace('ts', '')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is better to introduce more descriptive name for this variable (like baseName
)
@@ -12,10 +12,10 @@ import { getComponentPathname, typeOrder, repoURL } from 'docs/src/utils' | |||
|
|||
const pkg = require('../../../../package.json') | |||
const componentMenu = require('docs/src/componentMenu') | |||
const behaviorMenuItems = require('docs/src/componentMenuBehaviors') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather suggest to use behaviorMenu
as a file name (and behaviorMenu
as a variable name):
const behaviorMenu = require('docs/src/behaviorMenu')
This will result in consistent pattern with the line above:
const componentMenu = require('docs/src/componentMenu')
Couple of minor comments - please, take a look. Also, please, consider to review changes that were reported by Screener. Thank you! |
Accessibility description
Changes done in this PR:
Please see screenshot with changes: