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 unused angular directives #10169

Conversation

cillian
Copy link
Contributor

@cillian cillian commented Dec 16, 2022

What? Why?

This partially address #8700 and removes three unused angular directives.

  • The showMore directive was introduced here but it looks like it was never used, see a, b, c.
  • The fillVertical directive was introduced here but was no longer used after a3660df
  • The singleLineSelectors was no longer used after 532e27b

What should we test?

Check the directives are not called anywhere here...

https://github.com/openfoodfoundation/openfoodnetwork/search?q=fill-vertical
https://github.com/openfoodfoundation/openfoodnetwork/search?q=show-more
https://github.com/openfoodfoundation/openfoodnetwork/search?q=single-line-selectors

Release notes

Changelog Category: Technical changes

@cillian cillian self-assigned this Dec 16, 2022
@jibees
Copy link
Contributor

jibees commented Dec 16, 2022

# For now, this component is not being used.

7 years ago.

Thanks! 🙏 ☺️

Copy link
Contributor

@jibees jibees left a comment

Choose a reason for hiding this comment

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

Thanks!

I saw that:

// singleLineSelectors directive provides a drop-down that can overlap

What do you think?

@cillian
Copy link
Contributor Author

cillian commented Dec 16, 2022

Thanks @jibees what about this instead? c000010

We might be able to get rid of the CSS .filter-row class entirely, it's used here and here but I'm not 100% sure.

@jibees
Copy link
Contributor

jibees commented Dec 19, 2022

I removed that class and tested:

Capture d’écran 2022-12-19 à 10 43 01

Capture d’écran 2022-12-19 à 10 44 39

I've tested, and I can't see any difference...

So i've commit/pushed. Tell me what do you think about it!

c000010

@cillian
Copy link
Contributor Author

cillian commented Dec 22, 2022

Looks good to me, it doesn't seem to be used anywhere ✔️

Copy link
Contributor

@jibees jibees left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @cillian !

@filipefurtad0 filipefurtad0 self-assigned this Dec 28, 2022
@filipefurtad0 filipefurtad0 added the pr-staged-uk staging.openfoodnetwork.org.uk label Dec 28, 2022
@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Dec 28, 2022

Hey @cillian @jibees ,

I'm not sure what else would be needed from the testing side of things. I've had a quick smoke test around:

  • This commit seems to relate to /customers page: I've spotted no change in rendering or behavior
  • As pointed here, I've had a look at the shop front and different filter selections (shop and product categories) - all as before.

I'd say this is good to go? I'll more to ready to go, feel free to drop a line if anything else is needed on this one!

@filipefurtad0 filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Dec 28, 2022
@jibees
Copy link
Contributor

jibees commented Dec 29, 2022

Thanks @filipefurtad0 !
Since those directives were unused, it's rather difficult to know how to test ;)

I'd say Ready to go then!!

@sigmundpetersen sigmundpetersen merged commit d5ef683 into openfoodfoundation:master Jan 1, 2023
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.

4 participants