-
Notifications
You must be signed in to change notification settings - Fork 9
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
Doc generation #86
Doc generation #86
Conversation
fragsalat
commented
Jun 16, 2017
- Try to fix version generation by allowing circle ci to push to master
- Fixed generation of api doc
- Added documentation for date picker and notification
… component and fixed small bug
…s into doc-generation
…s into doc-generation
@@ -19,7 +19,7 @@ checkout: | |||
post: | |||
- git pull origin ${CIRCLE_BRANCH} | |||
- git clone git@github.com:wholesale-design-system/styleguide.git | |||
- git config --global user.email circleci@circleci | |||
- git config --global user.email t.schlage@gmx.net |
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.
maybe use different PRs
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.
in the future
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 say, in the future something like "tech-fabric@zalando.de" where all engineers working with it are subscribed ^^
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.
As I said you can create such a user if you want :D
I don't know where to store the credentials in a secure way so that also peoples after us get the access.
doc/components/date-picker.md
Outdated
<ws-date-picker | ||
value="2017-06-15" | ||
format="Y-m-d" | ||
options='{"minDate": "2017-06-01", "maxDate": "2017-06-24"}'> |
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.
should we change the format directly to min/max attributes like discussed?
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.
Yes but lets to this in a specific 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.
ok
doc/components/date-picker.md
Outdated
```html | ||
<ws-date-picker | ||
value="2017-06-15" | ||
format="Y-m-d" |
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.
do we need format?
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.
Actually it was a requirement by CAT and it makes sense because the backend requires specific date formats.
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 think the displaying of the date should be unified. @wholesale-design-system/ux
Processing the date afterwards to send it to the backend can be done by each application differently.
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.
We can align for Wholesale but since we want to give the styleguide to others we can not enforce them to use a specific formatting.
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.
discussed in #87
doc/components/dropdown.md
Outdated
@@ -26,6 +26,11 @@ like `text="Click me" icon="icon-filter"`. As following you can see an example o | |||
</div> | |||
</div> | |||
|
|||
**Disabled** | |||
As any common html element the dropdown can be disabled by adding the `disabled="1"` flag to 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.
disabled="1" is confusing. disabled is not really a boolean attribute: https://codepen.io/anon/pen/NgpZYe
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 recommend just using "disabled"
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 not working everywhere. The problem is that disabled will result in the property disabled: "" which will be false.
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.
is this a React thing?
if it is about implementation do the check props.disabled!==undefined
(or sth like that) this results in the same behaviour as native
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 a HTML and wrapper thing. If you call getAttribute('name') and you define disabled as a flag it will return an empty string which will be false. So either you define a list of boolean attributes or create the convention that every attribute which has an empty string will be boolean. Btw as far as I know disabled="1"
is valid HTML.
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 valid HTML but I just say it gives the wrong impression as disabled="false"
is still disabling a button: https://codepen.io/anon/pen/owwdEp
My approach is here https://codepen.io/anon/pen/GEEdXP
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.
Well, as you both discussed this I checked if disabled="1"
is really valid XHTML specification and it is not. If you use W3C Validator it shows you an error.
For example, setting the disabled attribute to the value "false" is disallowed, because despite the appearance of meaning that the element is enabled, it in fact means that the element is disabled (what matters for implementations is the presence of the attribute, not its value).
or
A number of attributes are boolean attributes. The presence of a boolean attribute on an element represents the true value, and the absence of the attribute represents the false value.
from here:
http://w3c.github.io/html/single-page.html#sec-boolean-attributes
so basically:
<input type="text" disabled />
<input type="text" disabled="" />
<input type="text" disabled="disabled" />
See: https://codepen.io/fokusferit/pen/gRoWKd (disabled="{0,1, lala, ...}" everything disables the button.
doc/components/notification.md
Outdated
The type of the event has to be `ws-notification-open` and the details has to be an object containing: | ||
- **title**: string, required | ||
- **description**: string, optional | ||
- **type**: info|error|warning|success, default: info |
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.
idk maybe mention those are strings?
doc/components/dropdown.md
Outdated
@@ -26,6 +26,11 @@ like `text="Click me" icon="icon-filter"`. As following you can see an example o | |||
</div> | |||
</div> | |||
|
|||
**Disabled** | |||
As any common html element the dropdown can be disabled by adding the `disabled="1"` flag to it. | |||
The style of the trigger will change and the dropdown con not be opened any more. |
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.
typo: con => can