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

remove-tab-key-access-to-home-logo #27

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

Conversation

RVany
Copy link
Contributor

@RVany RVany commented Feb 8, 2024

home logo link removed for all the users to eliminate tab key access to hidden home link in prod

@RVany RVany linked an issue Feb 8, 2024 that may be closed by this pull request
@RVany RVany changed the title two home logo link removed for all the users. remove-tab-key-access-to-home-logo Feb 14, 2024
@RVany RVany requested review from Steph4104 and doug0102 February 14, 2024 15:35
Copy link
Contributor

@doug0102 doug0102 left a comment

Choose a reason for hiding this comment

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

I think it would be better if we add the CSS to hide the logo into the adminSelectorsCSS, ownerSelectorsCSS, and memberSelectorsCSS. This way we don't hardcode anything and if we need to make a change it's simple to update instead of having to rebuild and redeploy the extension.

@RVany
Copy link
Contributor Author

RVany commented Feb 14, 2024

I think it would be better if we add the CSS to hide the logo into the adminSelectorsCSS, ownerSelectorsCSS, and memberSelectorsCSS. This way we don't hardcode anything and if we need to make a change it's simple to update instead of having to rebuild and redeploy the extension.

Ok. I understand that you recommend to maintain the use of property to hide and remove elements instead of hard coding it. I will change it and test.

@RVany RVany requested a review from doug0102 February 19, 2024 19:50
Copy link
Contributor

@Steph4104 Steph4104 left a comment

Choose a reason for hiding this comment

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

This fix cannot be apply to all site. Only to our main site. Community should not be impacted by this.

@RVany
Copy link
Contributor Author

RVany commented Feb 20, 2024

This fix cannot be apply to all site. Only to our main site. Community should not be impacted by this.

@Steph4104
As you know. This extension should apply to all sites. Currently, I can see that logo is hidden in community site as well. Are you going to display the logo back to community site?

@doug0102 Do you think any workaround for this?

@Steph4104
Copy link
Contributor

This fix cannot be apply to all site. Only to our main site. Community should not be impacted by this.

@Steph4104 As you know. This extension should apply to all sites. Currently, I can see that logo is hidden in community site as well. Are you going to display the logo back to community site?

@doug0102 Do you think any workaround for this?

Yes, I know the wall extension is apply to all site. But the fix you try to do should only apply to our main site. Not the user communities. I don't think the wall extension is the right place to apply this fix.

Currently, if you look at communities in PROD, logo icon is not hide
image

@RVany
Copy link
Contributor Author

RVany commented Feb 20, 2024

This fix cannot be apply to all site. Only to our main site. Community should not be impacted by this.

@Steph4104 As you know. This extension should apply to all sites. Currently, I can see that logo is hidden in community site as well. Are you going to display the logo back to community site?
@doug0102 Do you think any workaround for this?

Yes, I know the wall extension is apply to all site. But the fix you try to do should only apply to our main site. Not the user communities. I don't think the wall extension is the right place to apply this fix.

Currently, if you look at communities in PROD, logo icon is not hide image

ok. I will think if I can do any workaround for this.

@doug0102
Copy link
Contributor

doug0102 commented Feb 20, 2024

@doug0102 Do you think any workaround for this?

Maybe we could add another property to the extension that lets us specify a site ID and CSS that should only apply to that site. Something like siteIdCSS and we use a format like fe932622-1c74-48d2-8cd6-ee6a4c426ac4|div#spSiteHeader a[class^='logoWrapper'],div#spSiteHeader a[class^='shyLogoWrapper']. The | character could be used to separate each site/css entry and then the css would be parsed like in the other properties based on the , character.

Or we just hardcode it for our site assuming it never changes and we don't think we will need to do targeted CSS for any other sites.

@RVany
Copy link
Contributor Author

RVany commented Feb 20, 2024

@doug0102 Do you think any workaround for this?

Maybe we could add another property to the extension that lets us specify a site ID and CSS that should only apply to that site. Something like siteIdCSS and we use a format like fe932622-1c74-48d2-8cd6-ee6a4c426ac4|div#spSiteHeader a[class^='logoWrapper'],div#spSiteHeader a[class^='shyLogoWrapper']. The | character could be used to separate each site/css entry and then the css would be parsed like in the other properties based on the , character.

Or we just hardcode it for our site assuming it never changes and we don't think we will need to do targeted CSS for any other sites.

I am trying to prevent the hiding just for teams site. Lets see.

@RVany RVany requested a review from Steph4104 February 27, 2024 16:44
@RVany
Copy link
Contributor Author

RVany commented Feb 27, 2024

@Steph4104
Now the home logo is hided only in communication site so in all of our community sites' logo displays. When I debug, I found there are multiples of style tags creating by this extension so Shea @doug0102 recommends to make sure to maintain only one style tag and he made some changes for that. And he also recommends to use the property instead of hard coding the css selector. So we added a new property to add css selector only for communication site.

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.

Remove tab key access to home logo
3 participants