-
Notifications
You must be signed in to change notification settings - Fork 615
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
Fix issue #1136 (open correct product tabs) #1220
Conversation
Autotagging @bigcommerce/storefront-team @davidchin |
* Check for fragment identifier in URL requesting a specific tab | ||
*/ | ||
getTabRequests() { | ||
if (window.location.hash && window.location.hash.indexOf('#tab-similar') === 0 || window.location.hash.indexOf('#tab-warranty') === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not have hardcoded check for the hash in here. Bcoz there could be different ones e.g. http://cornerstone-light-demo.mybigcommerce.com/all/able-brewing-system/ is a cornerstone demo store in which we have #tab-related
which doesn't exist in the check. May be we just check if hash exists & .tabs
has a href with that hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@junedkazi #tab-related
is open by default, as is #tab-description
, which is why I'm only checking for the others. I guess removing the hardcoded checks would allow for future tabs or custom added ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it would support any new ones we plan to add in the future or any custom ones which a theme developer will add while working on their custom theme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we OK to go with
if (window.location.hash && window.location.hash.indexOf('#tab') === 0) {
to ensure we're actually requesting a tab and not, for instance, the reviews panel, i.e. #product-reviews
?
* Check for fragment identifier in URL requesting a specific tab | ||
*/ | ||
getTabRequests() { | ||
if (window.location.hash && window.location.hash.indexOf('#tab-') === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 I like this much better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some before & after screenshot or video
Also this PR might need a rebase. Let's hold off on doing it right away. Let's get the duplicate ids PR merged before we rebase this one. |
const $activeTab = $('.tabs').has(`[href='${window.location.hash}']`); | ||
const $tabContent = $(`${window.location.hash}`); | ||
|
||
$activeTab.find('.tab') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a guard in case someone uses a url fragment that doesn't actually match a tab element on the page?
@flair-duncan thanks for all the work on this PR. Do you have any availability to see this PR through ? This is really close. If you could just address Matt's comment & rebase the PR to fix the conflicts I think this PR is good to go. |
Closed in favor of #1304 |
What?
When the product page URL contains a fragment identifier that relates to tabbed content, i.e. #tab-similar or #tab-warranty, the correct tab should appear when the page loads.
Related Issue
Screenshots
Before
After