This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Remove sass styles and packages #1318
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openverse-bot
added
the
🚦 status: awaiting triage
obulat
force-pushed
the
add/sources_table
branch
from
6e01371 to
3b0f1c6
Compare
# Conflicts: # src/locales/po-files/openverse.pot # src/styles/vocabulary.scss
# Conflicts: # src/components/VSourcesTable.vue # src/locales/po-files/openverse.pot # src/styles/vocabulary.scss # tsconfig.json
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/1318 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
|
Size Change: -29 kB (-2%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
obulat
added
🟧 priority: high
sarayourfriend
suggested changes
May 3, 2022
There was a problem hiding this comment.
LGTM, just a couple things.
Can we also remove sass/scss entries from the following places, for consistency?
openverse-frontend/nuxt.config.ts
Line 182 in d6775ae
openverse-frontend/jest.config.js
Line 21 in df16c25
Also, the extension page layout is very broken now.
Other than that I couldn't find any issues 🎉
It does look like some of the layout shifted on the sources page and some other pages, but only very slightly. I agree they are minor and can be addressed later if there are discrepancies.
Comment on lines
-142
to
-145
| "sass": "^1.49.9", | ||
| "sass-loader": "^10.2.1", |
Ah, thank you for catching this! I did not look at the extension page. And thank you for flagging more places that still have |
|
It'd be nice to have simple snapshot tests for each of the content pages, like the sources ones. Though now that you've removed the global styles I'm much less worried about them suddenly changing without us noticing! |
# Conflicts: # src/locales/po-files/openverse.pot
obulat
commented
May 4, 2022
fcoveram
approved these changes
May 5, 2022
|
Wow, I did not expect such an easy approval from you, @panchovm! Thank you! |
|
I went through comparing the mockups and seeing if nothing is broken 😃 |
|
@sarayourfriend, I've added the snapshots for content pages. I wonder if I should click on the translation bar 'Close' button for 'rtl' snapshots. The translation banner normally should not change, but if it does, there will be so many snapshots to update! |
|
@obulat That probably does make sense, you're right. As long as we have tests specifically targeting the translation banner then I think it's a good idea. Once we have the UI state cookie in place we can simplify that as well by not having to click "close" on every test, we can just set the dismissed cookie at the top of the describe block. |
# Conflicts: # src/locales/po-files/openverse.pot
sarayourfriend
approved these changes
May 6, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes #774 by @sarayourfriend
Blocked by #1317Description
This PR removes all the legacy style files that used Sass. It also removes
sassandsass-loaderfrom dependencies.The Sources page was simplified a little bit, too.
I specifically added you to the reviewers list, @panchovm , because you can see style regressions much faster than anyone else :)
Testing Instructions
Open the app. There should be no drastic changes in the spacing, font sizes, button styles. Any inconsistencies with the mockups can be fixed in later PRs.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin