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

#60 Update landing page features #132

Conversation

nicoleiocana
Copy link
Collaborator

For issue #60.

All reviews requested by @CSchmitz81 from #131 have been resolved in this new PR.

Copy link
Member

@CSchmitz81 CSchmitz81 left a comment

Choose a reason for hiding this comment

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

Just a couple minor comments. I'll test the branch for comp matching since the things I mentioned shouldn't impact that much. Be forewarned though, that I'm pretty picky about comp matching, so it's not uncommon for me to have a lot of comments after the first test.

src/ctree-app/features-section.html Outdated Show resolved Hide resolved
src/ctree-app/features-section.html Show resolved Hide resolved
src/ctree-app/features-section.html Outdated Show resolved Hide resolved
@CSchmitz81
Copy link
Member

OK, I did more in depth testing of this and here's what I saw.

General

  1. First off, I realized the branch is actually named for THIS PR's number. The goal is actually to name it for the issue it's related to (i.e. Landing Page Features #60). The reason is we aren't guaranteed a PR number until we make the PR, but we want to make the branch before that. It's not worth changing now, but for future reference.
  2. Our convention is to prefix all component names with "ctree-", so "features-section.html"
  3. I'm not a fan of the image being directly tied to the name. In general it's a bad idea to tightly couple things which may not need to be. The human readable name should be required to be the same as the icon name in code. You should add a separate field for the icon name. Furthermore, I'd like to be able to use images instead of vectors if needed, so if you could add another property for the image name and set the iron-icon src field that would be great (it looks like it'll only be used if the icon name is empty).

Desktop

  1. The order of items doesn't match the comp, but it looks like the comp is inconsistent between mobile and desktop, so I'm fine with this. Just calling it out for completeness.
  2. The icons don't exactly match, but that's more of a detail which we can update later, so I'm fine with that too.
  3. The top/bottom section margins appear to be too small, and maybe the margin below the Features label.
  4. The horizontal padding between items is too small. The vertical padding may also need to be a bit larger.
  5. The vertical padding between the item label and item description is too large
  6. The item label text might be slightly too small, but the item description text is definitely too small. I'm not sure if that's also why the item description text looks a little gray instead of fully black, but it should be full black. You may also need to adjust the description text field size so the text doesn't wrap differently when you change the text size.

Mobile

  1. The Features section label text is too small
  2. The item label text is way too small and close to the icon
  3. The item description text may need to be slightly larger, and also double check to make sure the color is full black.
  4. The padding between the item label and item description needs to be slightly smaller
  5. I know the icons in the comp aren't perfectly aligned or consistently sized, but it looks like we might want to slightly increase
  6. The margin at the top/bottom of the features section needs to be increased, although at the top increasing the Features label size may increase this as well.

I also tested the way items flow when the screen size changes and that looks good to me.

Here are the images I used as reference, taken from the comp and in browser test
features_desk_comp
features_desk_test

features_mob_comp features_mob_test

@nicoleiocana
Copy link
Collaborator Author

As per Chris's request, below are the new images that are incorporated with his review(s):

pencil

shared-hands

magnifying-glass

looping-arrows

…e-demo into 132-update-landing-page-features
…ocana/ctree-demo into 132-update-landing-page-features
@CSchmitz81 CSchmitz81 linked an issue May 21, 2020 that may be closed by this pull request
@CSchmitz81 CSchmitz81 changed the title #132 update landing page features #60 Update landing page features May 21, 2020
@CSchmitz81
Copy link
Member

Merging latest changes because they're pretty close, but could still improve the PC browser version.

@CSchmitz81 CSchmitz81 merged commit a50cea5 into F4IF:develop-landing-page May 21, 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.

Landing Page Features
2 participants