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: adding a selectlist based solution switcher #168

Merged
merged 21 commits into from
May 19, 2021
Merged

fix: adding a selectlist based solution switcher #168

merged 21 commits into from
May 19, 2021

Conversation

ShreeSub
Copy link
Contributor

@ShreeSub ShreeSub commented Apr 22, 2021 •
edited

Description

With AEM branding changes now the switcher needs to be updated as the unified shell this is an
implementation for the same
changes will have to be added in foundation as well to be visible on the instance

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.

Pareesh
Pareesh approved these changes Apr 22, 2021
View reviewed changes
Copy link
Collaborator

@Pareesh Pareesh left a comment

Choose a reason for hiding this comment

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

LGTM

@Pareesh
Copy link
Collaborator

Pareesh commented Apr 22, 2021

Related to #158

@ShreeSub
Copy link
Contributor Author

ShreeSub commented May 4, 2021

@majornista @indra2gurjar can you please review this?

@ShreeSub
Copy link
Contributor Author

@indra2gurjar @majornista can you please review?

majornista
majornista requested changes May 17, 2021
View reviewed changes
Copy link
Collaborator

@majornista majornista left a comment •
edited

Choose a reason for hiding this comment

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

coral-shell-selectlistswitcher does not seem to populate the items in the list box correctly.

Looking at the rendered source for the menu, you can see that there is an empty coral-selectlist element, with no select list items that should have role="menuitem".

            <coral-shell-selectlistswitcher class="_coral-Shell-selectListSwitcher"><coral-selectlist handle="container" class="_coral-Shell-selectList-container _coral-Menu" role="listbox" aria-label="List" data-ol-has-click-handler=""></coral-selectlist>
              <coral-shell-switcherlist-item href="http://www.adobe.com/go/aem6_5_experiencecloud">Experience Cloud</coral-shell-switcherlist-item>
              <coral-shell-switcherlist-item href="http://www.adobe.com/go/aem6_5_advertisingcloud">Advertising Cloud</coral-shell-switcherlist-item>
              <coral-shell-switcherlist-item href="http://www.adobe.com/go/aem6_5_analytics">Analytics</coral-shell-switcherlist-item>
              <coral-shell-switcherlist-item href="http://www.adobe.com/go/aem6_5_audiencemanager">Audience Manager</coral-shell-switcherlist-item>
            </coral-shell-selectlistswitcher>

I think the problem is within the example: coral-shell-switcherlist-item elements should be coral-shell-selectlistswitcher-item.

majornista
majornista requested changes May 17, 2021
View reviewed changes
coral-component-shell/examples/index.html Outdated
Comment on lines 122 to 125
<coral-shell-switcherlist-item href="http://www.adobe.com/go/aem6_5_experiencecloud">Experience Cloud</coral-shell-switcherlist-item>
<coral-shell-switcherlist-item href="http://www.adobe.com/go/aem6_5_advertisingcloud">Advertising Cloud</coral-shell-switcherlist-item>
<coral-shell-switcherlist-item href="http://www.adobe.com/go/aem6_5_analytics">Analytics</coral-shell-switcherlist-item>
<coral-shell-switcherlist-item href="http://www.adobe.com/go/aem6_5_audiencemanager">Audience Manager</coral-shell-switcherlist-item>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there is a problem within the example: coral-shell-switcherlist-item elements should be coral-shell-selectlistswitcher-item.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made the necessary changes @majornista

@ShreeSub
fix: adding a selectlist based solution switcher
a2726a4 
With AEM branding changes now the switcher nneds to be updated as the unified shell this is an
implementation for the same
@ShreeSub
WIP - adding shellswitcherlist
85dae05
@ShreeSub
wip - modifying switcherlist component
7cd17e3
@ShreeSub
wip - updating switcherlist item
e3f5c69
@ShreeSub
wip - adding styles
e190d5c
@ShreeSub
wip - modifying selectlist switcher
e709ef1
@ShreeSub
adding support for selectlist-item
60687a7
@ShreeSub
correcting typo
a167166
@ShreeSub
wip: adding test cases
38e3d82
@ShreeSub
fix: added test cases and example for new switcher
fc21e64
@ShreeSub
fix: removing unnecessary comments
58a13e2
@ShreeSub
fix: incorporating review comments
690c589
@ShreeSub
fix: adding test cases
851837e
@ShreeSub
fix: adding more test cases
ecc7f66
@ShreeSub
fix: refactoring item name
15f9655
@ShreeSub
fix: refactoring class name
ab18b69
@ShreeSub
fix: incorporating review comments
171c2a5
@ShreeSub
fix: correcting test failures
df58773
@ShreeSub
fix: incorporating review comments
a8760a3
@ShreeSub
fix: incorporating review comments
d294f13
@ShreeSub
fix: correcting component name in index.html
3e2880e
@Pareesh
Copy link
Collaborator

Pareesh commented May 18, 2021

@ShreeSub , Please update the PR description

@Pareesh Pareesh merged commit 4651c4d into adobe:master May 19, 2021
github-actions bot pushed a commit that referenced this pull request May 19, 2021
@semantic-release-bot
chore(release): 4.10.22 [skip ci]
cd74a0a 
## [4.10.22](v4.10.21...v4.10.22) (2021-05-19)

### Bug Fixes

* adding a selectlist based solution switcher  ([#168](#168)) ([4651c4d](4651c4d))
* updating spectrum product logos according new branding changes ([#173](#173)) ([c55ca46](c55ca46))
@github-actions
Copy link

🎉 This PR is included in version 4.10.22 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@majornista majornista majornista approved these changes

@Pareesh Pareesh Pareesh approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ShreeSub @Pareesh @majornista