-
Notifications
You must be signed in to change notification settings - Fork 54
Conversation
Codecov Report
@@ Coverage Diff @@
## master #145 +/- ##
==========================================
- Coverage 88.93% 69.2% -19.74%
==========================================
Files 47 101 +54
Lines 768 1341 +573
Branches 103 251 +148
==========================================
+ Hits 683 928 +245
- Misses 83 411 +328
Partials 2 2
Continue to review full report at Codecov.
|
@@ -1,4 +1,5 @@ | |||
const fontAwesomeIcons = new Map([ | |||
['arrow right', 'f061'], |
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.
Icon used in quick start example.
@@ -86,6 +88,7 @@ const buttonStyles: IComponentPartStylesInput = { | |||
backgroundColor: typeSecondaryBackgroundColor, | |||
borderColor: typeSecondaryBorderColor, | |||
':hover': { | |||
color: typeSecondaryColor, |
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.
Fixed button styles when rendering as an a
tag.
@@ -1,3 +1,6 @@ | |||
import * as themes from './themes' | |||
export { themes } |
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.
Export themes from our package.
@@ -45,16 +46,19 @@ const semanticUIReactCompleter = { | |||
languageTools.addCompleter(semanticUIReactCompleter) | |||
|
|||
export interface IEditorProps extends AceEditorProps { | |||
id?: string | |||
id: string |
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 a DOM id and is required by AceEditor to work correctly.
@levithomason, can we also add an explanation, that Stardust UI consists of two parts:
This part is a little bit confusing for consumers of the library. I've seen that consumers create new components extended from |
@@ -45,6 +45,7 @@ const buttonStyles: IComponentPartStylesInput = { | |||
(iconPosition | |||
? { | |||
display: 'inline-flex', | |||
alignItems: 'center', |
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 discussed, there is a corresponding PR #135 where bunch of related Button
problems are addressed.
As a next work item related to that - the PR with conformance tests that will ensure that shorthand prop names correspond to component's anatomy parts, so that it would be intuitive for client to decide which name should be used for styling specific component's part:
<Button icon={...} styles={ icon: () => ... } />
@@ -6,6 +6,7 @@ | |||
"baseUrl": "../", | |||
"paths": { | |||
"@stardust-ui/react": ["src"], | |||
"@stardust-ui/react/themes/teams": ["src/themes/teams"], |
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.
👍
docs/src/components/CodeSnippet.tsx
Outdated
import * as React from 'react' | ||
import Editor, { EDITOR_BACKGROUND_COLOR } from './Editor' | ||
|
||
export interface ICodeSnippetProps { |
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.
here is a point for further thought: to me it seems a bit unreasonable to use 'I' prefix for interface types - this leads to the fiolloiwng situation where interface
types and alias type
types - both with exactly the same semantics, - are mixed:
import { ComponentVariablesInput, IComponentPartStylesInput } from '../../../types/theme'
Here we are importing two type interfaces (in general sense) for styles and variables - but for some reason one uses I
prefix :) What I would suggest is to omit I
prefix altogether for all types - this will make them consistent. Actually, the same approach React has taken for its types - quite a few of them are TS interfaces, but all the exposed names are consistent to the client as there are no I
prefixes
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 have no opinion here other than it should follow the best practice. It is nice to know when you can use extends
(interfaces but not types). However, it appears the official recommendation is to not use prefixes. Will update.
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 went to update this and it appears we have 44 interfaces in 24 files prefixed with "I" and only 4 interfaces in 3 files that aren't. I think we should make a separate PR that addresses this and updates the team about the new standard.
I will still update my usage here to not include the prefix.
docs/src/components/CodeSnippet.tsx
Outdated
} | ||
|
||
const CodeSnippet = ({ label = '', value, mode, style, ...rest }: ICodeSnippetProps) => { | ||
const editorMode: any = mode || label.replace(/.*\./, '') || 'jsx' |
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 seems that we might obtain not supported editor mode's value here (like tsx
from label
). Is it fine to pass this value to AceEditor then, or we should opt to some default (maybe, 'text') mode, so that only explicitly supported modes will be rendered (and, thus, guaranteed to be rendered properly)?
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.
True. This is partial left over from a previous iteration where label
was named filename
. However, the point still stands. I'm going to revert to using the less magical mode
prop just as the Editor does. This will ensure we can explicitly set the mode/highlighting separate from the label in the code snippet.
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.
You also asked if it is OK to pass an unsupported value, no. It will break the highlighting and also throw. Also, tsx
is not a supported mode.
The command should be 'yarn add @stardust-ui/react' yarn install doesn't work. |
The library @stardust-ui/react/themes/teams isn't installed by installing the core @stardust-ui/react and trying to add that module directly doesn't work. Where does one get that module from? Without it, the base example doesn't work. |
It is installed, however, it is not exported at that path until this PR is released. You can find it in |
@smykhailov Agreed but I think we need to flesh this out still. We haven't yet put forward a proposal for how consumers will do this. Let's save that for another PR. |
Non-SVG icons still don't seem to work with these instructions. They come in as boxes. Is there a need to include a font such as FontAwesome or something? |
This PR adds a little info to the Introduction page and links the Quick Start guide (many more guides on another branch will be posted tomorrow).