Showcase Page Edits

[jasonbariwix]

Please ignore index.js and index.module.css files sent in this PR. I am currently fixing the Homepage and this will be a separate PR, as requested!

I completed styling and functionality of the Showcase page to match the Glyph Figma:

1.Showcase page light and dark mode completed.
2. navbar items fixed.
3. footer fixed
4. cards and store buttons accounted for in both mode
5. created a Card.js file to move the function from Showcase.js so only one function per component
6. created footer icons for light mode and showcase stars png for both modes
7. added styling to all showcase icons and created a JSON file for cardsList.
8. added title to showcase browser tab,
9. made all cards same size and responsive
10. Added colorMode 'dark' to docusaurus.config.js file - this option will auto open the page in dark-mode unless this is overridden in the users browser settings.

[noomorph]

See my edits in c509dcc commit (currently reverted)

1. Using link1 and link2 instead of something more speaking about itself, e.g. linkGoogle and linkApple:
   
2. website/src/components/Showcase/Button.js looks suspiciously redundant. Do you really need it? @asafkorem check it twice with Jason.
3. Card.js was unnecessarily complicated. Use of <section> HTML5 element seems unjustified to me. I think using ul > li more fits the semantic meaning of this list.
4. useEffect with document.title change was irrational, because you implement it on a level of a component, not a page itself. Also, there is a better API, @docusaurus/Head (React Helmet, basically). Fixed.
5. All your CSS imports were directing to website/src/pages/showcase.module.css, even from the nested components. I had to split all the contents into small module CSS files, for every individual component.
6. The way you rendered images (from JSON) without require(...).default was not really correct. See the recommended way in Docusaurus: https://docusaurus.io/docs/static-assets#in-jsx Btw actually you had a correct way in the JS variant of the apps list.

[noomorph]

Needs changes. This is not my final review as I omitted reviewing CSS for now. Looks like it is a subject to change anyway. Just make sure to tidy it before the next review.

[asafkorem]

@jasonbariwix can you please link here to the original Figma?

[jasonbariwix]

Hi Yaroslav @noomorph , Thank you very much for your review of the PR and edits/suggestions. I really appreciate it and will start looking into them.

…

On Mon, Oct 17, 2022 at 9:49 AM Yaroslav Serhieiev ***@***.***> wrote: ***@***.**** requested changes on this pull request. Needs changes. This is not my final review as I omitted reviewing CSS for now. Looks like it is a subject to change anyway. Just make sure to tidy it before the next review. ------------------------------ In detox/android/detox/proguard-rules-app.pro <#3628 (comment)>: > @@ -1,4 +1,5 @@ -keepattributes InnerClasses, Exceptions + You shouldn't change this file. ------------------------------ In .vscode/settings.json <#3628 (comment)>: > @@ -0,0 +1,3 @@ +{ Add .vscode to .gitignore, please. ------------------------------ In website/package.json <#3628 (comment)>: > ***@***.***/react": "^1.6.21", ***@***.***/webpack": "^5.5.0", "clsx": "^1.1.1", "file-loader": "^6.2.0", "prism-react-renderer": "^1.2.1", "react": "^17.0.1", "react-dom": "^17.0.1", + "styled-components": "^5.3.5", Just to make sure, where do you use this lib in your code? ------------------------------ In website/src/components/Showcase/AppsList.js <#3628 (comment)>: > @@ -0,0 +1,88 @@ +const cardList = [ This card list looks better than the one you have defined in JSON. 🤔 ------------------------------ In website/src/components/Showcase/Button.js <#3628 (comment)>: > @@ -0,0 +1,13 @@ +import React from 'react'; + +import styles from './Button.module.css'; + +const Button = (props) => { Looks useless, you could just define a couple of CSS rules and render a simple <button> with them. Why define a class for such a trivial case? ------------------------------ In website/src/components/Showcase/Card.js <#3628 (comment)>: > @@ -0,0 +1,42 @@ +import React, { useEffect } from 'react'; +import styles from '../../pages/showcase.module.css'; +// import cardList from './AppsList'; +import cardList from '../../../showcase.json'; +import Link from ***@***.***/Link'; + +function Card({ title, image, link1, link2 }) { + return ( + <section> Monstrous markup. You should be fine just with li > (div > img) + span + a + a, 3 levels at most. Or even li > img + span + a + a 2 levels. ------------------------------ In website/src/components/Showcase/Showcase.js <#3628 (comment)>: > @@ -0,0 +1,34 @@ +import React, { useEffect } from 'react'; +import Card from './Card'; +import styles from '../../pages/showcase.module.css'; +// import cardList from './AppsList'; +import cardList from '../../../showcase.json'; + +function Showcase() { + useEffect(() => { Incorrect place to use. You should use it not in component but on your page. Use @theme/Head instead of useEffect ------------------------------ In website/src/pages/index.js <#3628 (comment)>: > @@ -13,10 +13,10 @@ function HomepageHeader() { <header className={clsx('hero hero--primary', styles.heroBanner)}> <div className="container"> <h1 className="hero__title">{siteConfig.title}</h1> - <p className="hero__subtitle">{siteConfig.tagline}</p> + <h2 className="hero__subtitle">{siteConfig.tagline}</h2> I disagree with changing p to h2. See more: https://stackoverflow.com/a/3910470 ------------------------------ In website/src/pages/index.js <#3628 (comment)>: > @@ -50,6 +49,11 @@ function HomepageSubHeader() { ); } +export function Section({ element = 'section', children, className, background = 'light' }) { Unused code, probably. ------------------------------ In website/src/pages/showcase.js <#3628 (comment)>: > +import clsx from 'clsx'; +import Layout from ***@***.***/Layout'; +import Link from ***@***.***/Link'; +import useDocusaurusContext from ***@***.***/useDocusaurusContext'; +import styles from '../pages/showcase.module.css'; +import StandWithUkraine from '../components/StandWithUkraine'; +import Showcase from '../components/Showcase/Showcase'; + +function ShowcaseHeader() { + const { siteConfig } = useDocusaurusContext(); + + return ( + <header className={clsx('hero hero--primary', styles.heroBanner)}> + <div className="container"> + <h1 className="hero__title">Users Showcase</h1> + <h2 className="hero__subtitle">Check out who is using Detox to Gray box test their React Native Apps</h2> See my comment above regarding h2 (https://stackoverflow.com/a/3910470) ------------------------------ In website/src/pages/showcase.module.css <#3628 (comment)>: > @@ -0,0 +1,338 @@ +/** Lots of copy&paste. Please remove unrelevant parts. ------------------------------ In website/src/components/Showcase/Card.js <#3628 (comment)>: > @@ -0,0 +1,42 @@ +import React, { useEffect } from 'react'; +import styles from '../../pages/showcase.module.css'; Use individual module css. Why do you use shared css file for all components? ------------------------------ In website/src/components/Showcase/Showcase.js <#3628 (comment)>: > @@ -0,0 +1,34 @@ +import React, { useEffect } from 'react'; +import Card from './Card'; +import styles from '../../pages/showcase.module.css'; Use individual module css. Why do you use shared css file for all components? ------------------------------ In website/src/pages/showcase.js <#3628 (comment)>: > @@ -0,0 +1,39 @@ +import React, { Component } from 'react'; +import clsx from 'clsx'; +import Layout from ***@***.***/Layout'; +import Link from ***@***.***/Link'; +import useDocusaurusContext from ***@***.***/useDocusaurusContext'; +import styles from '../pages/showcase.module.css'; Why not just ./showcase.module.css? Why go ../pages? ------------------------------ In website/src/pages/showcase.js <#3628 (comment)>: > @@ -0,0 +1,39 @@ +import React, { Component } from 'react'; +import clsx from 'clsx'; +import Layout from ***@***.***/Layout'; +import Link from ***@***.***/Link'; +import useDocusaurusContext from ***@***.***/useDocusaurusContext'; +import styles from '../pages/showcase.module.css'; +import StandWithUkraine from '../components/StandWithUkraine'; You could use @site/src/components/StandWithUkraine instead. — Reply to this email directly, view it on GitHub <#3628 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AWGQHSXYFKK6DGARZARZJXTWDTZF5ANCNFSM6AAAAAARETXPFA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[jasonbariwix]

https://www.figma.com/file/QGhhB7djq0OxvLzULhs7u5/Detox?node-id=160%3A5254 Here is the original Figma link @asafkorem - Glyph just finished providing the correct rgba values for the styleguide section Frame 29. Didn't have this info, prior to my PR.

…

On Tue, Oct 18, 2022 at 12:27 PM Asaf Korem ***@***.***> wrote: @jasonbariwix <https://github.com/jasonbariwix> can you please link here to the original Figma? — Reply to this email directly, view it on GitHub <#3628 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AWGQHSW3YLI6BOEHRDIDQ3TWDZURVANCNFSM6AAAAAARETXPFA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[asafkorem]

@jasonbariwix, you did an excellent job! 👏🏼 This is truly mind-blowing!! 🤯

I've added the Showcase application form link.

It looks like you resolved all the comments made by Yarik (👑 🙌🏼), and stuff looks much better now!
Please verify this and resolve them on GitHub before I'll merge that

[jasonbariwix]

Hi Asaf! Thank you very much 🙂! Its been a great project to be a part of and thank you for all your support. I still need to resolve all of Yarik's comments and will do so as soon as I can. Can't wait to see it live!!

…

On Fri, Oct 28, 2022, 12:31 PM Asaf Korem ***@***.***> wrote: @jasonbariwix <https://github.com/jasonbariwix>, you did an excellent job! 👏🏼 This is truly mind-blowing!! 🤯 I've added the Showcase application form link. It looks like you resolved all the comments made by Yarik, and stuff looks great now! Please verify this and resolve them on GitHub before I'll merge that [image: ] — Reply to this email directly, view it on GitHub <#3628 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AWGQHSTEMJCKVGWS4CFPIGTWFOMPXANCNFSM6AAAAAARETXPFA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[jasonbariwix]

@asaf Korem - Thanks again and I just resolved all comments made by Yarik!

…

On Fri, Oct 28, 2022 at 12:31 PM Asaf Korem ***@***.***> wrote: @jasonbariwix <https://github.com/jasonbariwix>, you did an excellent job! 👏🏼 This is truly mind-blowing!! 🤯 I've added the Showcase application form link. It looks like you resolved all the comments made by Yarik, and stuff looks great now! Please verify this and resolve them on GitHub before I'll merge that [image: ] — Reply to this email directly, view it on GitHub <#3628 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AWGQHSTEMJCKVGWS4CFPIGTWFOMPXANCNFSM6AAAAAARETXPFA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[asaf]

Probably the wrong Asaf :)