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

Feat navbar improves #183

Merged
merged 4 commits into from
Jan 19, 2024
Merged

Feat navbar improves #183

merged 4 commits into from
Jan 19, 2024

Conversation

flavioriper
Copy link
Contributor

@flavioriper flavioriper commented Jan 12, 2024

Description

We need this to change the navbar to a new version with cards and columns

Mobile version still the same

@flavioriper flavioriper marked this pull request as draft January 12, 2024 15:53
Copy link

github-actions bot commented Jan 12, 2024

Visit the preview URL for this PR (updated for commit a73d81e):

https://estuary-marketing--pr183-feat-navbar-improves-h3fjr09u.web.app

(expires Sat, 17 Feb 2024 11:17:46 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 76f6b095a0752e5d9c6c890267f9fdc3e392161e

@flavioriper flavioriper self-assigned this Jan 12, 2024
@flavioriper flavioriper marked this pull request as ready for review January 12, 2024 18:28
@flavioriper flavioriper requested a review from jshearer January 12, 2024 18:30
@jshearer
Copy link
Collaborator

jshearer commented Jan 16, 2024

I haven't seen the mocks for this, but overall it looks good! A few nitpicks:

Screen Shot 2024-01-16 at 13 22 43
This bottom text looks quite small

Screen Shot 2024-01-16 at 13 22 46
Feels like these shouldn't be indented?

Screen Shot 2024-01-16 at 13 22 50
This image is pretty blurry

@jeffatestuary
Copy link

new build incoming to fix any applicable updates @jshearer

Copy link
Collaborator

@jshearer jshearer left a comment

Choose a reason for hiding this comment

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

This is looking good from a presentation standpoint -- it looks way better than the navbar that I hacked together in the beginning!

That being said, there are a few places where this introduces hard-coding of "content" concerns that should really be handled in Strapi. If we're chomping at the bit to get this out then I say let's merge and fast-follow with the updates to de-hardcode these, otherwise really they should be fixed before merging.

},
]

export const compare = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should pull from Strapi like the current comparison links in the navbar do

]

export const caseStudies = [
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should also pull from Strapi so when we publish more case studies they automatically show up

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this isn't used anywhere?

Copy link
Collaborator

@jshearer jshearer Jan 18, 2024

Choose a reason for hiding this comment

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

Love that this is an SVG, but I would also love for us to not be hardcoding "content" like this. Does it make sense to add a field to the case studies model in Strapi for this? What would that field be called? I'm happy to go add it if we come up with a good framing here

@jeffatestuary
Copy link

jeffatestuary commented Jan 18, 2024 via email

Copy link
Collaborator

@jshearer jshearer left a comment

Choose a reason for hiding this comment

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

Let's merge with #183 (review) as a fast-follow

@jshearer jshearer merged commit d076e00 into master Jan 19, 2024
2 checks passed
@jshearer jshearer deleted the feat-navbar-improves branch January 19, 2024 19:31
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