Skip to content
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

Issue 21 product showcase #86

Merged
merged 19 commits into from
Feb 20, 2020
Merged

Issue 21 product showcase #86

merged 19 commits into from
Feb 20, 2020

Conversation

nkapolcs
Copy link
Contributor

@nkapolcs nkapolcs commented Feb 3, 2020

Issue #21
This section is to showcase products that are not open source or have a bigger scope so they cannot be showcased as git repositories.

Changes:

  • create settings entry
  • create products, productDisplayer components
  • use MUI Card component
  • create basic layout
  • create a basic display to the card
  • create online presence buttons
  • use pictures as heading
  • delete learn more
  • technologies icons
  • recreate the layout: fit 4-5 columns in a row on full-screen
  • rewrite and separate sc components
  • add optional settings logic
  • update documentation
  • place products below TCC
  • rename ProductDisplayer
  • fallback for socialIcons

Visual:

Peek 2020-02-05 23-21

@nkapolcs nkapolcs changed the title Issue 21 product showcase [Wip] Issue 21 product showcase Feb 3, 2020
@nkapolcs nkapolcs self-assigned this Feb 3, 2020
@thisismydesign
Copy link
Member

thisismydesign commented Feb 4, 2020

Looks cool :)

Adjusted a few requirements:

  • Let's make it 1 row, N columns
    • example picture below
    • use pictures as heading for the cards (similar to mui example)
    • use pictures with ~7:3 ratio (do not constrain ratio but ~7:3 examples make sense)
    • fit 4-5 columns in a row on full-screen
  • technologies are in the same row as the name, aligned to the right

Additionally:

  • no need for learn more link, let's replace that with online presence icons aligned to the left

Examples:

image

image

@nkapolcs
Copy link
Contributor Author

nkapolcs commented Feb 4, 2020

  • Let's make it 1 row, N columns

    • example picture below
    • use pictures as heading for the cards (similar to mui example)
    • use pictures with ~7:3 ratio (do not constrain ratio)
    • fit 4-5 columns in a row on full-screen
  • technologies are in the same row as the name, aligned to the right

Additionally:

  • no need for learn more link, let's replace that with online presence icons aligned to the left

Oh i misunderstood the logo, thank for clarifying :)

@nkapolcs
Copy link
Contributor Author

nkapolcs commented Feb 5, 2020

There is a design decision, and I'm curious about your opinion:
So we have two options how we display the product grid, the first:

localhost 3000 (9)

We have fix grid element width as you can see above, and the second:

localhost 3000 (8)

Where we always fill the width of the container, no matter there is one grid element in the row or four.
Which one to choose?

@nkapolcs nkapolcs changed the title [Wip] Issue 21 product showcase Issue 21 product showcase Feb 5, 2020
@thisismydesign
Copy link
Member

Looks great!

Few comments regarding display:

  • Since there're pictures involved let's stick with your first proposed option. Then 4 products would be the limit, right?
  • Place products below TCC

I like the decision to match the height of the boxes to the biggest text. Created a follow-up for that: #88 (but don't worry about it for now).

src/components/Products/Products.js Outdated Show resolved Hide resolved
src/components/Products/Products.style.js Outdated Show resolved Hide resolved
src/components/Products/Products.style.js Outdated Show resolved Hide resolved
@nkapolcs
Copy link
Contributor Author

nkapolcs commented Feb 6, 2020

  • Since there're pictures involved let's stick with your first proposed option. Then 4 products would be the limit, right?

Yes in one row 4 is the max (5 was too small).

Example:
localhost 3000 (10)

@gomorizsolt
Copy link
Collaborator

Btw, sorry for not reviewing the last few PRs, I was unsubscribed from the notifications for some reason. :)

Copy link
Collaborator

@gomorizsolt gomorizsolt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Left two comments.

src/components/App/App.js Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
thisismydesign
thisismydesign previously approved these changes Feb 10, 2020
Copy link
Member

@thisismydesign thisismydesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix Zsolt's outstanding comment, otherwise approved from my side.

@nkapolcs nkapolcs merged commit ac040d2 into master Feb 20, 2020
@nkapolcs nkapolcs deleted the issue-21_product-showcase branch February 20, 2020 15:34
@nkapolcs nkapolcs mentioned this pull request Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants