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

Refine Title Module UI - fixes issue #762 #763

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

NARUDESIGNS
Copy link
Collaborator

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

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt jasmine
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • PR body includes fixes #0000-style reference to original issue #
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

@NARUDESIGNS
Copy link
Collaborator Author

I'm sorry I think there's a mistake, it was meant to be just 2 commits and not 10. I created a new branch and made just 2 commits to cover all the changes for this issue but I don't know how it pushed the older commits from the older branches that have been merged already.
Do I have to delete the branches after they've been merged?
Please guide me on how to do it right.
@TildaDares

@TildaDares
Copy link
Member

@NARUDESIGNS I use this gist https://gist.github.com/jordanbtucker/17b6c3c7cdaca12327d0 when I have unwanted commits in my PR. <commit> will be the latest commit hash (the last commit you added to this branch). The <branch> on line 2 will be main and the one on line 3 will be the name of this branch. I hope this helps. Let me know if you face any more issues.

@NARUDESIGNS
Copy link
Collaborator Author

@TildaDares thank you, I'd try that.
But going forward, how do I prevent it from happening?
Is it that I need to pull first on every new branch before making changes, committing and pushing?

@TildaDares
Copy link
Member

Just make the branch you’re working on is updated with recent changes.

@NARUDESIGNS
Copy link
Collaborator Author

Just make the branch you’re working on is updated with recent changes.

Alright thanks, I think that's the problem.
Also, do you mind if I just close this PR and open a new one? I'm a bit worried about using rebase.

@TildaDares
Copy link
Member

@NARUDESIGNS Doing a rebase is safe but if you’re worried you can go ahead and open a new one

@NARUDESIGNS
Copy link
Collaborator Author

@NARUDESIGNS I use this gist https://gist.github.com/jordanbtucker/17b6c3c7cdaca12327d0 when I have unwanted commits in my PR. <commit> will be the latest commit hash (the last commit you added to this branch). The <branch> on line 2 will be main and the one on line 3 will be the name of this branch. I hope this helps. Let me know if you face any more issues.

I was evaluating this. The last 2 commits are the only commits I intended for this PR. If I do git reset --hard <commit> on the last commit, the other commits will remain since they came before the last commit. If I do it on any other commit, the new commit will be discarded and I stand a chance to loose my changes.
From my past experience using git reset <commit> it points to the specified commit and discards other commits that comes afterwards.

@TildaDares
Copy link
Member

git rebase allows you to pick and drop commits.

@NARUDESIGNS
Copy link
Collaborator Author

@NARUDESIGNS Doing a rebase is safe but if you’re worried you can go ahead and open a new one

Hi @TildaDares so I'm trying this out (first time using rebase anyway). From line 2 in the guide you shared, I did

git rebase -i upstream/main

I get this error

fatal: invalid upstream 'upstream/main'

Am I doing it wrong? am I supposed to just do

git rebase -i main

?

@TildaDares
Copy link
Member

@NARUDESIGNS I think it’s supposed to be the name of your branch. My bad!

@NARUDESIGNS
Copy link
Collaborator Author

NARUDESIGNS commented Dec 15, 2021

@NARUDESIGNS I think it’s supposed to be the name of your branch. My bad!

I still get the error when I put my branch name.

fatal: invalid upstream 'upstream/refine-ple-title-module-UI'

Should I use upstream or origin?
I think upstream should be the original PublicLab repo but origin should be my forked version right?
@TildaDares

@TildaDares
Copy link
Member

@NARUDESIGNS I just ran the same command on my local repo and it worked

Screenshot 2021-12-15 at 13 19 04

@NARUDESIGNS
Copy link
Collaborator Author

@TildaDares what branch were you on when you did this?
Main? or the affected branch (in my case refine-ple-title-module-UI)?

@TildaDares
Copy link
Member

@NARUDESIGNS I was on a branch I just created

@NARUDESIGNS
Copy link
Collaborator Author

@NARUDESIGNS I was on a branch I just created

Same here, I wonder why I'm getting that error. You also forked the PublicLab.editor repo which is where you are creating these branches and making these changes right?

@TildaDares
Copy link
Member

@NARUDESIGNS Yes

@TildaDares
Copy link
Member

@NARUDESIGNS This article might help https://luisdalmolin.dev/blog/branched-off-the-wrong-branch-in-git/

@NARUDESIGNS
Copy link
Collaborator Author

Thanks, let me check it out.
@TildaDares

@gitpod-io
Copy link

gitpod-io bot commented Dec 15, 2021

@NARUDESIGNS
Copy link
Collaborator Author

It worked!!! 🥳
Thank you so much @TildaDares
I used git cherry-pick <commit> since it allowed me to pick the commits I want.
I will appreciate it if you can share with me some tips on how you find useful resources for your issue 🙏

@TildaDares
Copy link
Member

@NARUDESIGNS I’m glad that article helped you out.

I don’t really have special tricks I use for finding resources, I just search google. Just make sure the search text is descriptive. For example, when I was searching for a resource to help you out, I just entered How to fix a branch that was branched from a feature branch into the search bar.

@NARUDESIGNS
Copy link
Collaborator Author

Thank you, I've learnt a lot from our conversation and this issue as well.
Please help review the changes and let me know what I have to do. It is the second week already and I can't wait to start checking items off my list.

Also I wanted to ask, after a PR has been merged, it is safe to delete the branch locally right?

@TildaDares
Copy link
Member

@NARUDESIGNS Yes it’s safe to delete the branch after it has been merged.

@TildaDares
Copy link
Member

@NARUDESIGNS Can you add a link to the issue this PR is resolving?

@NARUDESIGNS
Copy link
Collaborator Author

Great!
Thanks. I look forward to your feedback on this PR. Let me know if you need screenshots or anything at all you need me to revisit or provide.
@TildaDares

@NARUDESIGNS
Copy link
Collaborator Author

NARUDESIGNS commented Dec 15, 2021

@NARUDESIGNS Can you add a link to the issue this PR is resolving?

Here it is #762
It is from my planning task list. I used that button you discovered to convert the task into an issue.

@TildaDares
Copy link
Member

@NARUDESIGNS Can you add before and after pics of the UI change?

@TildaDares
Copy link
Member

TildaDares commented Dec 15, 2021

It doesn’t have to be now. You can do it later.

@NARUDESIGNS
Copy link
Collaborator Author

Here
TitleModule

TitleModule-mobile

@TildaDares
Copy link
Member

I’ll send a review in the morning. Thanks!

@NARUDESIGNS
Copy link
Collaborator Author

I’ll send a review in the morning. Thanks!

Thank you very much! @TildaDares

@TildaDares
Copy link
Member

@NARUDESIGNS Can you help me understand why you think this change should be made to the page?

@NARUDESIGNS
Copy link
Collaborator Author

Good morning @TildaDares I made some explanations about the changes in my proposal.
I thought we needed to be descriptive with the headers and then center the layouts. On smaller screens you can see tips squeezed together on the left (for modules that has tips). This realignment will solve that issue.
Please take an overview of my proposal again to see UI changes I intend to work on.
Thanks.

@TildaDares
Copy link
Member

Hi @NARUDESIGNS, your proposal has a lot of information in it so it's a lot easier for everyone if you can just give a short description of the issue (with images if need be). Thanks Paul!

@TildaDares
Copy link
Member

So I've gone through your proposal for this issue and this is what I think

I think one of the reasons why numbers were chosen for this page was to give a kind of step-by-step instruction to the user on how to create a post. The first thing they notice on the page are the numbers which serve as a visual guide. The text underneath each number already gives enough description to the input elements without drawing attention away from it.

The text you added draws too much attention away from the important elements. If we're going to add text, it needs to be at the background because this page is more about action than content.

I like where this idea is coming from but I don’t think it would work on this page. I'm not a UI/UX expert so maybe @jywarren could chime in on this.

@NARUDESIGNS
Copy link
Collaborator Author

NARUDESIGNS commented Dec 16, 2021

Thank you for your opinion and review. Myself, I'm not a UI/UX designer as well but here are my own opinions.
When I opened the editor, I assumed I don't know what public lab is except I could use it to write and add content.
About the numbers being a guide, I got to this page with the mind of a typical user and wasn't sure what to do at step 5

Screenshot 2021-12-16 at 09 30 34

Also, the numbers seem like this is what you need to do first and this is what you need to do next when ideally I could just input my title in step1, skip all the way and add my content in step4 and I'd be able to publish. This was why I thought it was important to be descriptive in the header instead.

@NARUDESIGNS
Copy link
Collaborator Author

NARUDESIGNS commented Dec 16, 2021

For the alignments and centring of elements, have a look at these screenshots and the tips on mobile view

Screenshot 2021-12-16 at 09 31 31

Screenshot 2021-12-16 at 09 31 47

These are steps 2 and 4. You can see the tips tightly packed onto the left on step 2 and then step 4 weirdly describing what it does or what it is meant to do.
These are just my opinions anyway and I'm open to feedback on how we can progress here.
Thank you always for your quality time @TildaDares

@jywarren
Copy link
Member

Hi all! I appreciate your thoughtfulness on this!

So i think first it's really helpful to state the goals as discretely as possible - i see a few here:

  1. Fix issue: labels/guides are horizontally squashed on mobile
  2. Fix issue: numbers take a lot of space and maybe aren't that helpful?
  3. Help explain what step 5 is about to a new user

I also recognize what @TildaDares is saying about the big Add Title text - it seems too big and takes a lot of page space, and it's also redundant with the "placeholder" text within the input itself (the greyed out text that disappears as you begin to type)

I think we could approach this first by doing some more mockups, rather than jumping into code too quickly, so we come to agreement about how things should look even before starting to code.

But once we like what we see, the PR implementation could be done in several PRs - like one for the titles, one for the horizontal squashing, etc.

We might also benefit from looking at some other editors and how they render in small screens. What about Gmail's Android app for composing an email? Or Twitter's editor? Other examples, maybe like WordPress? What do we like? This is changing a really key page for the community so any major changes we may want to get buy-in from community reps, as well. But don't be overwhelmed - this should be a fun process!

That said, this does seem to be embarking on a pretty large set of design issues. Are we ready for this so early in the project, and are we otherwise on track with the testing issues so far? Given that the above set of design changes may take some time to work on thoroughly in a collaborative and transparent way, maybe it's really good to start early, so by the time other issues are resolved, we've had plenty of time to think about and discuss these design changes.

Anyways, i hope this is helpful! Great start here and please don't hesitate to ask more questions!

@NARUDESIGNS
Copy link
Collaborator Author

Thank you very much @jywarren truly we shouldn't jump into code just yet. So I'm going to have to come up with some mock designs maybe with Figma or Adobe XD to show some UI changes, then we can agree on what works and what doesn't.

@jywarren
Copy link
Member

jywarren commented Dec 20, 2021 via email

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