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

Make profile settings save button sticky when modified #9378

Merged
merged 7 commits into from
Jul 22, 2020

Conversation

emgoto
Copy link
Contributor

@emgoto emgoto commented Jul 18, 2020

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

When a user modifies a field on the profile settings page, the save button footer will become "sticky" along the bottom of the screen (see GIF below).

Related Tickets & Documents

This is for issue #9240

QA Instructions, Screenshots, Recordings

sticky

Added tests?

  • yes
  • no, because they aren't needed
  • no, because I need help

Happy to add a test - just not sure how or where! (contributor added tests)

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

alt_text

Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

Looks cool! A couple of details I highlighted in my review

app/javascript/packs/stickySaveFooter.js Show resolved Hide resolved
app/javascript/packs/stickySaveFooter.js Outdated Show resolved Hide resolved
app/assets/stylesheets/settings.scss Show resolved Hide resolved
@rhymes rhymes requested a review from nickytonline July 20, 2020 08:21
@emgoto
Copy link
Contributor Author

emgoto commented Jul 20, 2020

Thanks @rhymes! I made the changes you suggested. I'm surprised the precommit hook didn't pick up on the new line stuff.

I have a PR etiquette question - is it considered polite/normal to use the "re-request review" feature? And am I okay to close off your comments as I address them? (I usually use Bitbucket for code reviews so some of this is new to me).

Edit: Another question, when a contributor has an issue assigned to them but there's no activity on it, is there a certain amount of time we can let pass before we double-check if we can pick up the issue instead? (A month?)

Copy link
Contributor

@msarit msarit 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, @emgoto! 👏

You had asked for some help with tests? If you check the file spec > system > user > user_edits_profile_spec.rb, you'll see where you can add a spec testing your code.

I wrote something up real quick that you can use as a guide 😄
I chose to update the "Website Url" field, but you can choose whichever field to update.
Let us know if you have any questions!

require "rails_helper"

RSpec.describe "User edits their profile", type: :system do
  let(:user) { create(:user, saw_onboarding: true) }

  before do
    sign_in user
    visit "/settings/profile"
  end

  describe "visiting /settings/profile" do
    ...
    ...
    ...

    it "makes the 'Save Button' footer sticky once a field is filled in", js: true do
      expect(page).not_to have_css(".sticky-save-footer")
      
      fill_in "user[website_url]", with: "example.com"
      find("#user_website_url").native.send_keys :tab   # this un-focuses the filled-in field
      
      expect(page).to have_css(".sticky-save-footer")
    end
  end
end

@rhymes
Copy link
Contributor

rhymes commented Jul 21, 2020

@emgoto

I have a PR etiquette question - is it considered polite/normal to use the "re-request review" feature? And am I okay to close off your comments as I address them? (I usually use Bitbucket for code reviews so some of this is new to me).

definitely yes to both :)

Edit: Another question, when a contributor has an issue assigned to them but there's no activity on it, is there a certain amount of time we can let pass before we double-check if we can pick up the issue instead? (A month?)

We don't have a set amount of time but if you find an issue you'd like to take over and see the maintainer hasn't responded in a while please leave a comment so we can reassign it.

Thank you!

@rhymes rhymes force-pushed the emgoto/9240-sticky-settings-save branch from cf4dced to b1e4163 Compare July 21, 2020 07:02
@rhymes
Copy link
Contributor

rhymes commented Jul 21, 2020

@emgoto I rebased your commits on top of master and force pushed to fix the failing flaky tests

@emgoto emgoto requested review from rhymes and msarit July 22, 2020 11:16
Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

Thanks @emgoto!

We're looking in the failing specs which seem to be unrelated with the content of your PR

@rhymes rhymes merged commit f6ee0f2 into forem:master Jul 22, 2020
@ludwiczakpawel
Copy link
Contributor

Hey @emgoto I know it's merged already but just wanted to say "thanks"! Really really nice 👍

Btw. do you think we could add this for other settings pages as well? At least for /settings/ux since this one, I think, is visited and updated pretty often :)

@emgoto
Copy link
Contributor Author

emgoto commented Jul 23, 2020

Yes, that would be cool! Do we need a separate issue for it, or I can just raise a PR against the same issue (#9240)?

@ludwiczakpawel
Copy link
Contributor

@emgoto no need for separate issue, just a PR would be enough :)

@emgoto
Copy link
Contributor Author

emgoto commented Jul 23, 2020

I've raised the PR for settings/ux. I've chosen a theme in the past and navigated away, not realising there was even a save button so hopefully this will help other people from making that same mistake!

I think for any other pages that we want to add this to, it would make a great candidate for being an issue tagged with good first issue, as any new devs would be able to check out my PR to see how to do it.

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.

None yet

4 participants