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.

Sign up for GitHub

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

Jump to bottom

Fix open Contact tab from "customers only" message #11789

Merged
merged 2 commits into from
Dec 19, 2023
Merged

Fix open Contact tab from "customers only" message #11789

merged 2 commits into from
Dec 19, 2023

Conversation

duleorlovic
Copy link
Contributor

@duleorlovic duleorlovic commented Nov 9, 2023 •
edited by rioug

What? Why?

We need stimulus attributes that will activate Contact tab

What should we test?

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

rioug
rioug approved these changes Nov 9, 2023
View reviewed changes
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Nice one ! Thanks for your help.
I noticed you merged master into your branch, we prefer rebasing the dev branch on master to keep the git history nice an clean.

dacook
dacook approved these changes Nov 10, 2023
View reviewed changes
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

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

Thank you!

That code seems quite wordy, but I see that it's the same method as used elsewhere. Hopefully one day we might be able to simplify it..

@duleorlovic
Copy link
Contributor Author

duleorlovic commented Nov 10, 2023 •
edited

Nice one ! Thanks for your help.
I noticed you merged master into your branch, we prefer rebasing the dev branch on master to keep the git history nice an clean.

@rioug Sorry, I have clicked on the github page that I want to rebase on master... Now I have rebased locally and force pushed, so now there is only one commit

@duleorlovic
Copy link
Contributor Author

Thank you!

That code seems quite wordy, but I see that it's the same method as used elsewhere. Hopefully one day we might be able to simplify it..

@dacook you are right. We should not wait for refactoring. I added a another commit that solves this comment , mainly we do not need to use two methods, just data-action="tabs-and-panels#activate".

Removed shop-tabs controllers since we can listen on "data-action": "orderCycleSelected@window->tabs-and-panels#activateDefaultPanel"

Added test for cases:
* activate by clicking on tab
* activateDefaultPanel on orderCycleSelected event
* activateFromWindowLocationOrDefaultPanelTarget to activate tab based on achor in URL

@dacook @rioug Please review again

rioug
rioug approved these changes Nov 15, 2023
View reviewed changes
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Looks great ! thanks for cleaning that up 🙏

spec/javascripts/stimulus/tabs_and_panels_controller_test.js
Comment on lines +126 to +128
Object.defineProperty(window, "location", {
value: new URL("http://example.com/#non_valid_panel"),
configurable: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a lot cleaner than the previous solution !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, no need to use mock

@mkllnk
Copy link
Member

mkllnk commented Nov 16, 2023

There are a couple of specs failing.

@sigmundpetersen
Copy link
Contributor

@duleorlovic did you want to fix the failing specs?
Then this can move forward 👍

@duleorlovic duleorlovic force-pushed the 11784_contact_tab branch 5 times, most recently from b61aaf4 to bd05966 Compare December 15, 2023 21:53
@duleorlovic duleorlovic force-pushed the 11784_contact_tab branch 2 times, most recently from 994d197 to b844e09 Compare December 16, 2023 06:24
@duleorlovic
Use activate instead of changeActivePanel/ChangeActiveTab
120e299 
Remove shop-tabs controllers since we can listen on `"data-action":
"orderCycleSelected@window->tabs-and-panels#activateDefaultPanel"`

Test for cases:
* activate by clicking on tab
* activateDefaultPanel on orderCycleSelected event
* activateFromWindowLocationOrDefaultPanelTarget to activate tab based
  on achor in URL
@duleorlovic
Copy link
Contributor Author

There are a couple of specs failing.

Sorry for late reply. I was thinking that there were som Flaky test, but I had to update some test case also.
I used data-test=link_for_enterprise_fees instead of id=enterprise_fees so it is obvious that we are using for expect(page).to have_selector "[data-test=link_for_enterprise_fees]"

@mkllnk mkllnk changed the title Add data-action=tabs-and-panels to activate Contact tab Fix open Contact tab from "customers only" message Dec 17, 2023
mkllnk
mkllnk approved these changes Dec 17, 2023
View reviewed changes
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@drummer83 drummer83 self-assigned this Dec 18, 2023
@drummer83 drummer83 added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Dec 18, 2023
@drummer83 drummer83 assigned drummer83 and unassigned drummer83 Dec 18, 2023
@drummer83 drummer83 added the no-staging-UK A tag which does not trigger deployments, indicating a server is being used label Dec 19, 2023
@drummer83
Copy link
Contributor

Hi @duleorlovic,
Thank you for this contribution! I have tested this PR on our staging server and it seems to work great! 💪

I have tested that

  • with the 'registered users only' option enabled
    • guest users see the contact message and clicking the contact link works well, as well as the login link,
    • logged-in users (no customers yet) see the contact message and clicking the link works well,
    • logged-in customers see the products right away,
  • with the 'registered users only' option disabled
    • guest users see the products right away,
    • logged-in users (no customers yet) see the products right away,
    • logged-in customers see the products right away.

Here is a short video (covering only parts of the tests):

Peek.2023-12-19.14-49.mp4

I think this one ready to go! 🥳
I'll go ahead and merge this one! 🚀
Thanks again!

@drummer83 drummer83 merged commit 4973b5c into openfoodfoundation:master Dec 19, 2023
54 checks passed
@drummer83 drummer83 removed the no-staging-UK A tag which does not trigger deployments, indicating a server is being used label Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkllnk mkllnk mkllnk approved these changes

@rioug rioug rioug approved these changes

@dacook dacook Awaiting requested review from dacook

Assignees

@duleorlovic duleorlovic

@drummer83 drummer83

Labels
None yet
Projects
OFN Delivery board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Contact tab does not open on click on link that changes anchor
6 participants
@duleorlovic @mkllnk @sigmundpetersen @drummer83 @dacook @rioug