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

Create comm of practice page (new PR) #1250

Merged
merged 18 commits into from
Mar 18, 2021

Conversation

csseu
Copy link
Contributor

@csseu csseu commented Mar 18, 2021

Addressing #678
Started by @dmcneary and passed on to me.

Notes:

  • Cards are a _communities collection processed using Liquid. The template.md won't be shown
  • _config.yml updated to include _communities and I couldn't help myself from alphabetizing the entries
  • Two media calls used in the header css to deal with the uncanny tablet valley where it just didn't look good as two columns (this wasn't in the Figma so it's ad-libbed)
  • The .svg splash files were original but I thought they might be better as smaller jpgs. I didn't delete the .svgs because I wasn't sure, maybe the team would rather use the svgs?

If you're not seeing any card content: In my experience with Jekyll/Docker, occasionally it won't load changes from collections and _config.yml, even when I start with a fresh server. My workaround was to dump _site in the trash and have jekyll serve everything new. (I'm wondering if that was the issue with the Liquid tag for img dir as well).

@csseu
Copy link
Contributor Author

csseu commented Mar 18, 2021

I think I'm supposed to include screenshots

desktop
mobile
tablet

@akibrhast
Copy link
Member

  • The projects/<project-name> page is broken . I believe this is happening due to some changes in _config.yml file
Broken `projects/project-name`

image

  • The template.md file is not doing anything, that I can tell. If the purpose of it is to document what a communities.md files should like I recommend moving it to the wiki.
  • Having time in a string format in data source really isn't good practice and will cause headaches down the road. Can we get it from VRMS? Have it in a UNIX format -- > meeting-times: Thursdays 8:00-9:00 pm PT (every week)

Copy link
Member

@akibrhast akibrhast left a comment

Choose a reason for hiding this comment

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

Please ensure that the projects/ page is not broken .

Also If i am not mistaken this PR should be closing #678. Please link the issue to this PR

@csseu
Copy link
Contributor Author

csseu commented Mar 18, 2021

Thanks for the fast response @akibrhast!

  • Yes - found extra lines in _config.yml and removed them. Seems to work for me now.
  • Deleted the _template - we'll need to update this according to the date/VRMS issue and place in wiki when done
  • VRMS - I don't know how this works yet. I'll dig into it when I have time, or can you/someone advise?

@csseu csseu linked an issue Mar 18, 2021 that may be closed by this pull request
16 tasks
@akibrhast akibrhast self-requested a review March 18, 2021 17:36
@akibrhast
Copy link
Member

lgtm 👍 . Merging

@akibrhast akibrhast merged commit e54eb8a into hackforla:gh-pages Mar 18, 2021
@daniellex0
Copy link
Member

daniellex0 commented Mar 19, 2021

Hey @akibrhast should this issue have been closed already? Considering @ruqpyL2 's question above about VRMS... And since this is a brand new page, maybe there should be a chance for a second dev reviewer to take a look just in case.

The design looks great @ruqpyL2, awesome job- there are some CSS fixes like the gray background instead of beige which I could have asked to fix before this went live, but I can add the notes in a separate issue for someone else. But I just wanted to note there is no need to rush this so much @akibrhast lol, I want to make sure everything is cleared before this can be connected to the site.

@akibrhast
Copy link
Member

maybe there should be a chance for a second dev reviewer to take a look just in case.

You are right. I do apologize for that. The reason I merged this is because from what I could tell it seemed like this PR addressed all the necessary changes requested in the issue for the most part. Also, since the page is not linked yet and did not break anything I figured everyone in the team ui/ux,pm,developers would be easily able to access the page without needing to pull it and discover bugs/css changes needed and create new issues regarding this page . #678

Considering @ruqpyL2 's question above about VRMS

Her question regarding VRMS was in regards to my initial comment regarding vrms. Which I realized was an unnecessary statement to make that had nothing to do with this PR or the issue this PR would be closing.

there are some CSS fixes like the gray background instead of beige which I could have asked to fix before this went live,

It's live but not linked yet anyhwere in the page. Just like the about page. Which is live and going through iterative development/fixes . Maybe the css fixes could be iterative and changing the background color could be a good second issue?

But I just wanted to note there is no need to rush this so much @akibrhast

You are right. I am really sorry about that. Didnt think through when I was merging.

I want to make sure everything is cleared before this can be connected to the site

It's not connected to the site yet as far as I am aware. Is there somewhere on the site that is linking to hackforla.org/communities-of-practice ?

@daniellex0
Copy link
Member

@akibrhast Ok got it, no worries- it's true there is the option to let it go live since it is not yet connected to the site, and iterate on that as we have with the About Us page (even though this page is smaller). But in the future if that's the intention, can you please note that before you merge? It takes more work to create more separate issues for the fixes, but that's ok. It's good to have extra issues.

And regarding VRMS, should the meeting times be automated then or not? Do you mean that will need to be on a separate issue?

@akibrhast
Copy link
Member

Understood.

And regarding VRMS, should the meeting times be automated then or not? Do you mean that will need to be on a separate issue?

Yes it should. Yes it should be its own issue. Which would highly be dependent on vrms.. and may or may not be addressed any time soon. For now, hardcoding it is the right way to go.

@daniellex0
Copy link
Member

@akibrhast Got it, thank you for the clarification Akib! Glad we got this all sorted lol

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.

Communities of Practice Page
4 participants