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

Feature/UI updates #94

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Feature/UI updates #94

wants to merge 7 commits into from

Conversation

amdomanska
Copy link
Collaborator

@amdomanska amdomanska commented Dec 3, 2024


UI updates:

  1. Update Footer content
  2. Placeholder in Inputs changed to correct colour
  3. Warning buttons colour changed
  4. Dropdown menu layout fixed
  5. Burger menu icon incorrect animation turned off
  6. Text in inputs to be black

Developer Checklist

Developers should review and confirm each of these items before requesting review

  • Code meets acceptance criteria from issue
  • Unit tests are written and all pass
  • User Test Scripts (if required) are written and have been run through
  • Code documentation and related non-code documentation has all been updated

Reviewer Checklist

Reviewers should review and confirm each of these items before approval
If there are multiple reviewers, this section can be duplicated for each reviewer

  • Code meets acceptance criteria from issue
  • Unit tests are written and all pass
  • User Test Scripts (if required) are written and have been run through
  • Code documentation and related non-code documentation has all been updated
  • Migation has been created and tested

Testing

List user test scripts that need to be run

List any non-unit test scripts that need to be run

@amdomanska amdomanska mentioned this pull request Dec 3, 2024
@J4bbi J4bbi force-pushed the feature/ui_updates branch from be52e12 to 42f67ab Compare December 3, 2024 16:08
@J4bbi J4bbi requested a review from cc-a December 9, 2024 10:30
Copy link
Collaborator

@cc-a cc-a left a comment

Choose a reason for hiding this comment

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

I'm afraid I'm having trouble testing this one. I don't see any of the implemented changes after running invenio-cli assets build or invenio-cli assets watch. No improvement from clearing my virtual environment (pipenv --rm) or building and running the web app from scratch in a container (invenio-cli container --build).

I get the following after invenio-cli assets build which may be related:

WARNING in css/3526.8d0ebe6ac24735b4490c.css
css/3526.8d0ebe6ac24735b4490c.css from Css Minimizer plugin
postcss-calc:: Lexical error on line 1: Unrecognized text.

Erroneous area:
1: auto + 5%
^..^ webpack://./less/invenio_app_rdm/theme/elements/container.overrides:126:6
Warning: css/3526.8d0ebe6ac24735b4490c.css from Css Minimizer plugin
postcss-calc:: Lexical error on line 1: Unrecognized text.

Erroneous area:
1: auto + 5%
^..^ webpack://./less/invenio_app_rdm/theme/elements/container.overrides:126:6
at CssMinimizerPlugin.buildWarning (/home/ccaveayl/.local/share/virtualenvs/fair-data-repository-wS1pO6pL/var/instance/assets/node_modules/css-minimizer-webpack-plugin/dist/index.js:299:26)
at /home/ccaveayl/.local/share/virtualenvs/fair-data-repository-wS1pO6pL/var/instance/assets/node_modules/css-minimizer-webpack-plugin/dist/index.js:599:55

webpack 5.97.1 compiled with 15 warnings in 42765 ms
Built webpack project.

@amdomanska
Copy link
Collaborator Author

@cc-a I have found the error that caused some of the changes to not show, sorry for that! It all now should work fine. I also added the fix for #83 (review)

Regarding the warning: this is not related. I can see the same warning on develop and it doesn't affect correct assets build.

This is ready for re-review.

@amdomanska amdomanska requested a review from cc-a December 19, 2024 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants