PROD-2309 Dev Center -> dev #235
Conversation
brooketopcoder
left a comment
brooketopcoder
left a comment
There was a problem hiding this comment.
@billsedison This is great work!
99% of the suggestions are specific to the Platform UI or to my preferences for being able to easily read code, which are totally expected for a first PR.
One issue that blocked me from verifying changes locally was this error that prevents the build from succeeding:
Please let me know if you have any questions, concerns, etc re any of these suggestions.
src-ts/tools/dev-center/dev-center-lib/CopyButton/CopyButton.tsx
Outdated
Show resolved
Hide resolved
...ter-pages/community-app/landing-page/dev-center-articles-section/ArticleCard/ArticleCard.tsx
Outdated
Show resolved
Hide resolved
...v-center/dev-center-pages/community-app/landing-page/dev-center-articles-section/Articles.ts
Outdated
Show resolved
Hide resolved
...v-center/dev-center-pages/community-app/landing-page/dev-center-articles-section/Articles.ts
Outdated
Show resolved
Hide resolved
...ter-pages/community-app/landing-page/dev-center-articles-section/CardSection/CardSection.tsx
Outdated
Show resolved
Hide resolved
...ter-pages/community-app/landing-page/dev-center-articles-section/CardSection/CardSection.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@billsedison Thanks for making all those changes. I know some were really tedious, so I doubly appreciate it.
There are just a handful of super minor suggestions, then we should be able to deploy to dev.
We have 2 other features in dev now, so we'll need to coordinate to make sure that we don't cause any issues w/those.
Also, please make sure the branch builds yarn build. It looks like we might have missed an instance of the pad-xxx not getting renamed.
| .title { | ||
| @include font-barlow-condensed; | ||
| @include font-weight-semibold; | ||
| font-size: 34px; |
There was a problem hiding this comment.
let's use styles from the design library rather than creating new styles for a specific module:
|
|
||
| > h2 { | ||
| padding-right: $pad-xxl; | ||
| background: url('../../assets/i/icon-cheveron-up.svg') no-repeat center right 0 / auto; |
There was a problem hiding this comment.
We should only be using icons from the heroicons library, which makes it difficult to get a specific URL for an icon to use as a bg image path.
I'm not seeing a reason this SVG needs to be a bg image, though.
|
|
||
| return ( | ||
| <header className={styles['header']}> | ||
| <h1 className={styles['title']}>{title}</h1> |
There was a problem hiding this comment.
when i said the "header" tags, i was referring to the "h1" tag. sorry for the miscommunication.
please remove the module-specific styling from the h1 and h2 tags
| padding: $pad-lg 0 0; | ||
| } | ||
|
|
||
| .title { |
There was a problem hiding this comment.
this still needs to be addressed
| } | ||
|
|
||
| &:first-of-type:not(:first-child) { | ||
| margin-top: 58px; |
There was a problem hiding this comment.
this still needs to be addressed
| } | ||
|
|
||
| export default ArticleCard | ||
| /* tslint:disable:cyclomatic-complexity */ |
There was a problem hiding this comment.
This can easily be broken out into helper functions. Pleased fix the complexity.
There was a problem hiding this comment.
when i said the "header" tags, i was referring to the "h1" tag. sorry for the miscommunication.
please remove the module-specific styling from the h1 and h2 tags
But if we remove the styling then the styling will be broken.
Expected
If we remove the styling, then it becomes
This is a special styling for the header of Getting Started Guide, as all content of the getting started guide is from the Markdown text.
There was a problem hiding this comment.
Yeah, it doesn't look good. This is the scenario I mentioned in a prior comment...
We need to use the names of styles that are provided in the mockup. So if the mockup says this is an "h2" tag but has it styled completely different from an h2 tag, then either the mockup is incorrect and needs to be changed, or the design system needs to change its definition of an h2 globally.
I just got a chance to see the mockups in figma for the first time, and it looks like the "Getting Started Guide" has the font named "overline." So we should use a div with the class "overline."
i understand that h2 tags have meaning in terms of SEO and accessibility, but we have not addressed that with the UX team yet.
…tation, reformat the code
brooketopcoder
left a comment
brooketopcoder
left a comment
There was a problem hiding this comment.
@billsedison once again, great work.
I am going to go ahead and approve this PR, even though we didn't get the h2 tag styling resolved.
thanks for such quick turnaround.
|
PLEASE DO NOT MERGE |
brooketopcoder
changed the title
brooketopcoder
changed the title
|
I have confirmed with the team that this is ready to deploy to dev, and the dev env is not in use with any other initiatives. |
No description provided.