Skip to content
This repository was archived by the owner on Oct 6, 2025. It is now read-only.

Conversation

@IvanKiral
Copy link
Contributor

Motivation

Which issue does this fix? Fixes #issue number

Migration and rebranding

Checklist

  • Code follows coding conventions held in this repo
  • Automated tests have been added
  • Tests are passing
  • Docs have been updated (if applicable)
  • Temporary settings (e.g. variables used during development and testing) have been reverted to defaults

How to test

If manual testing is required, what are the steps?

@netlify
Copy link

netlify bot commented Aug 5, 2022

Deploy Preview for kontent-sample-app-vue ready!

Name Link
🔨 Latest commit df1fb68
🔍 Latest deploy log https://app.netlify.com/sites/kontent-sample-app-vue/deploys/630c665702ce64000980d473
😎 Deploy Preview https://deploy-preview-101--kontent-sample-app-vue.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@IvanKiral IvanKiral marked this pull request as ready for review August 5, 2022 07:22
@Simply007 Simply007 self-requested a review August 8, 2022 07:34
Copy link
Contributor

@Simply007 Simply007 left a comment

Choose a reason for hiding this comment

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

Notes:

  • package.json - rename name of the packages (remove kontent- from package name)
  • Remove GT-Walfheim and use OS font from the guide
  • (optional) take a look to packages (try at least minor versions bump)

Great work with the Store rewrite! 👍🏼

@IvanKiral IvanKiral requested a review from Simply007 August 8, 2022 10:34
@Simply007
Copy link
Contributor

Upgrade will be done in #102

Copy link
Contributor

@Simply007 Simply007 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 unify the configuration page with new visuals (logo colors)
  • Add redirects to netlify: https://github.com/kontent-ai/sample-app-react/blob/master/netlify.toml
  • Try to unify font to WorkSans - if something does not look OK - try to adjust font-weight
  • Coffees filter does not work
  • Brewers filter is missing
  • I don't se subtitle on Home hero banner, Titles on Latest articles
  • Rich text resolution for twitter widget and video is not resolved ~/en-us/articles/cf106f4e-30a4-42ef-b313-b8ea3fd3e5c5

@IvanKiral IvanKiral requested a review from Simply007 August 11, 2022 08:11
Copy link
Contributor

@Simply007 Simply007 left a comment

Choose a reason for hiding this comment

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

If i go to some page and back - i don't see the cafes:

recording

If I go to https://deploy-preview-101--kontent-sample-app-vue.netlify.app/es-ES/articles/117cdfae-52cf-4885-b271-66aef6825612 (ES language) the link in text does not preserve nor load currect language varian of the coffee (link is https://deploy-preview-101--kontent-sample-app-vue.netlify.app/coffees/kenya-gakuyuni-aa)

@IvanKiral
Copy link
Contributor Author

I agree with the first one, there was one additional condition which made this error. The second one might be due to fallbacking into English language due to not published content in ES language.

@Simply007
Copy link
Contributor

Simply007 commented Aug 15, 2022

I agree with the first one, there was one additional condition which made this error. The second one might be due to fallbacking into English language due to not published content in ES language.

In that case, there should be ˙https://deploy-preview-101--kontent-sample-app-vue.netlify.app/en-US/coffees/kenya-gakuyuni-aa˙ (with language codename right). The thing is we don't want to have the same content on different URLs. Not sure about the fallback - If we want to change the language prefix or keep it - but some should be present.

@IvanKiral IvanKiral requested a review from Simply007 August 15, 2022 10:36
Copy link
Contributor

@Simply007 Simply007 left a comment

Choose a reason for hiding this comment

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

Everything, but language behavior is fine.

  • lowercase language prefixes
  • default route should enforce prefix /articles -> /en-us/articles
  • If a fallback is used enforce the fallback language right to the prefix and basically change it
  • Describe the behavior in the README -> at least brief - to be ready

@IvanKiral IvanKiral requested a review from Simply007 August 18, 2022 06:14
Copy link
Contributor

@Simply007 Simply007 left a comment

Choose a reason for hiding this comment

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

  • Homepage link for About us has a "More Articles" label.

Haven't found anything more.

@IvanKiral IvanKiral requested a review from Simply007 August 22, 2022 11:33
Copy link
Contributor

@Simply007 Simply007 left a comment

Choose a reason for hiding this comment

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

Update the readme to reflect the current state.

@IvanKiral IvanKiral force-pushed the migration branch 3 times, most recently from 406789f to c7fb0bc Compare August 23, 2022 12:12
@IvanKiral
Copy link
Contributor Author

I have updated the readme. Added a new paragraph about data fetching with an example, fixed grammar mistakes, and adjusted the structure of the whole readme

@IvanKiral IvanKiral requested a review from Simply007 August 23, 2022 12:44
Copy link
Contributor

@Simply007 Simply007 left a comment

Choose a reason for hiding this comment

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

Great work on README.

I have one comment about configuration + I might change the name of the kk-logo.svg to something more accurate.

@Simply007 Simply007 self-requested a review August 29, 2022 09:52
@Simply007 Simply007 merged commit 5530c67 into master Aug 29, 2022
@Simply007 Simply007 deleted the migration branch August 29, 2022 09:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants