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

V2 functions connect #431

Merged
merged 10 commits into from
Jul 7, 2022
Merged

Conversation

stephaniequintana
Copy link
Collaborator

This branch is a combination of the added functionality that @Forchapeatl has completed and an updated UI for the sandbox page of Infragram.org. It includes the changes addressed in #427 and #421 and has multiple additional changes. A live representation can be viewed at https://stephaniequintana.github.io/infragram/index2.html#

We agree that this branch provides a solid foundation to build upon as our projects progress. It contains changes to the

  • index2.html,
  • infragram2.css and the
  • infragram2.js files.

Moving forward, we would like to merge this branch and then make further changes to the files directly through individual PRs.

We would very much appreciate feedback, questions, ideas...as this PR holds multiple commits and encompasses many new features,

  • Local video processing
  • Resolution Selection
  • Canvas Recording
  • multiple interface functions
  • and an updated Mobile layout

We look forward to you thoughts!

Screen Shot 2022-07-06 at 8 31 34 PM

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

@gitpod-io
Copy link

gitpod-io bot commented Jul 7, 2022

@jywarren
Copy link
Member

jywarren commented Jul 7, 2022

This looks great and I love the progress so far! A lot of the input i have is very focused on one thing or another, so maybe best for after we merge this? For example:

  • i wonder if we should put the "source" buttons (webcam, upload) actually on the pane in the center, instead of saying "through your computer above". Or, we could have them be in both places. Right now, my eye is drawn to Quick Start and (naively, imagining self as a new user) i click it without visually "getting" that i need to choose a source first.
  • I'm still zooming in 2x in my browser to get text to be a "comfortable" size, so I wonder if we should increase the default still (see below)

But overall this is looking great, i'm happy to merge if you're both ready?

image

@stephaniequintana
Copy link
Collaborator Author

@jywarren, thank you for the feedback &&& I agree.

As is, it is not inherently clear how to best utilize the site/tools. I cannot locate the specific comment, but in an earlier version @TildaDares made some great suggestions on how to better convey the initial steps. Also, I think adding in the welcome/navigation tour and labeling the "source" icons will help immensely. I am still working out the best flow for myself and am hoping that the UI will improve as I progress.

To the font-size, there are/were issues with the card-container itself overfilling the image-container on resize (both resizing the font and the window), but I have had an epiphany as to why this is occurring. Taking in @TildaDares's suggestions and also possibly moving the current welcome text to the welcome/navigation tour, I'll be able to address the font size with no issues.

I believe we are ready to merge this. I know that @Forchapeatl is already working on correcting a toggle function and it would be great if she will be able to open a PR for that directly against this.

Moreover, I understand that this specific PR still has issues of its own. We will very much depend on feedback to help us identify some of those as well as the overall best approach for our users.

Thanks again, @jywarren, I'm excited to see this develop!

@jywarren jywarren merged commit 9a9c035 into publiclab:main Jul 7, 2022
@jywarren
Copy link
Member

jywarren commented Jul 7, 2022

OK, exciting!!!! Great work everyone!!

@stephaniequintana
Copy link
Collaborator Author

stephaniequintana commented Jul 7, 2022

Oof, @jywarren, I just realized I was a bit careless in what I had pushed to this v2_functions_connect branch...there is a file, indexF.html which does not belong. My apologies.

If I can correct this, please let me know how - I believe it can only be deleted by someone with write access to the repo. Again, I apologize for overlooking this.

@jywarren
Copy link
Member

jywarren commented Jul 7, 2022 via email

@stephaniequintana
Copy link
Collaborator Author

Yes, that. I don't understand how to do that. If I submit a PR, a request to merge a branch (with that file removed), then when merged with the main branch that file will still exist bc it's already in the main branch (?)

Perhaps my brain is tired and not processing correctly. It is my understanding that the file can only be removed from the CLI with someone who has access to the repo.

You are suggesting that if I git rm indexF.html and open a PR, merging that will in effect accomplish removing it from the main branch?

@Forchapeatl
Copy link
Collaborator

Forchapeatl commented Jul 7, 2022

Hello @stephaniequintana I think this is the easiest way
selection

@stephaniequintana
Copy link
Collaborator Author

@Forchapeatl, thank you. Unfortunately, that avenue will only delete the file in my own personal fork. If I do this while on a new branch and open a PR for that branch then the file will be removed from main when merged?

Ugg. I, guess this is one way to learn (not my preferred way, btw) - thank you all for your patience.

@jywarren
Copy link
Member

jywarren commented Jul 7, 2022

If I do this while on a new branch and open a PR for that branch then the file will be removed from main when merged?

Yes, that's right! Sorry i didn't see your message, was offline writing for a bit.

stephaniequintana added a commit to stephaniequintana/infragram that referenced this pull request Jul 27, 2022
* connected image-upload function

* npm github pages

* npm UNinstall github pages

* working draft, connecting functions

* saving WIP

* eliminate bootstrap5 reference

* added infragram2.js - CSS WIP

* Proceccing, Resolution, Recording - Forchapeatl patch 4 (#6)

* Update index2.html

* Update infragram2.js

* Update infragram2.css

* update css & video-controls placement

* v2 to merge with PL

Co-authored-by: FORCHA <[email protected]>
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