search

EssamWisam / cmp-docs

A comprehensive guide for prospective, current and past students in the computer engineering department of Cairo university.
https://cmp-docs.pages.dev
52 stars 8 forks source link

docs: Add class of 2020 Arabic and English yaml files #52

Closed OsamaNabih closed 1 month ago

vercel[bot] commented 1 month ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
cmp-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 17, 2024 0:56am
netlify[bot] commented 1 month ago

Deploy Preview for cmp-docs ready!

Name Link
Latest commit 695808506bde6a3b9783f3795f0e6d96c224fd18
Latest deploy log https://app.netlify.com/sites/cmp-docs/deploys/664753c69235660008c91669
Deploy Preview https://deploy-preview-52--cmp-docs.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 configuration.

Iten-No-404 commented 1 month ago

@OsamaNabih, Thank you for your time and valuable contribution. I will run the LinkedIn scraping script on the added file before merging.

EssamWisam commented 1 month ago

@OsamaNabih, Thank you for your time and valuable contribution. I will run the LinkedIn scraping script on the added file before merging.

Thank you, Iten. I was discussing with Eng. Osama something regarding the markdown field because he contacted me regarding it so let's wait to resolve this before merging.

Thoughts are either to drop the markdown field in all yamls and let it be the constant You can know more about Ahmed and reach him out by visiting his [LinkedIn]() or to keep it for added flexibility (e.g., a class wanting to write something different or someone in that class wants to write their bio). In the latter case, we can also let You can know more about Ahmed and reach him out by visiting his [LinkedIn]() to be constant and markdown to show below it.

Iten-No-404 commented 1 month ago

Thank you, Iten. I was discussion with Eng. Osama something regarding the markdown field because he contacted me regarding it so let's wait to resolve this before merging.

Sure. I already noticed this issue and did a small fix for it.

Thoughts are either to drop the markdown field in all yamls and let it be the constant You can know more about Ahmed and reach him out by visiting his [LinkedIn]() or to keep it for added flexibility (e.g., a class wanting to write something different or someone in that class wants to write their bio). In the latter case, we can also let You can know more about Ahmed and reach him out by visiting his [LinkedIn]() to be constant and markdown to show below it.

I say we keep the markdown key for flexibility and add a check that adds the LinkedIn link in the format similar to what we are used to if something else is written in it. You can the logic below. I have already committed it a couple of minutes ago but I didn't push it yet in case there's any other pushes on Osama's end. For example:

I already implemented the above since errors occurred when opening the modals.

I also finished running the script on the C2020.yaml and added a few students that were present in one file (ex: Eng. version) but missing in the other. Lastly, I added C20XX.yaml and C20XX_ar.yaml as templates.

@OsamaNabih, please let me know if you are going to push any more commits, if not, then I will push the changes I have done. @EssamWisam, let me know your thoughts as well.

EssamWisam commented 1 month ago

Thanks for the impressive work @Iten-No-404. I assume your last case also handles the case where the markdown field doesn't exist at all (equivalent to being empty). If so, I like this resolution but we should document the behaviour and whole idea of LinkedIn feature. Once we merge this, I will do this in a separate PR and tag you.

Iten-No-404 commented 1 month ago

Thanks for the impressive work @Iten-No-404.

Don't mention it, @EssamWisam.

I assume your last case also handles the case where the markdown field doesn't exist at all (equivalent to being empty).

Yes, that is handled as well as other similar cases. I just didn't mention all the possibilities for brevity.

If so, I like this resolution but we should document the behaviour and whole idea of LinkedIn feature.

Document it as in the README file? It needs some modifications indeed.

Once we merge this, I will do this in a separate PR and tag you.

Sure, no problem.

OsamaNabih commented 1 month ago

I agree with @Iten-No-404 suggestion's, thanks for the work. Looks good to me. You can proceed. Sorry about the inconsistency between the 2 YAML files.

Iten-No-404 commented 1 month ago

I agree with @Iten-No-404 suggestion's, thanks for the work. Looks good to me. You can proceed. Sorry about the inconsistency between the 2 YAML files.

It wasn't much of an issue, don't worry about it. Thank you again for your contribution. 🫡