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.
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
Add search button to header #3483
base: main
Are you sure you want to change the base?
Add search button to header #3483
Changes from 23 commits
b021a18
7bf2beb
4d81fc4
b0eb10b
586e897
672fa08
6b43a75
b375a29
7fb0698
289b12b
700de62
f6c0fef
5378d07
4cc084c
b1bf012
b3b5c22
82fc26d
8335a2e
0a4cfe4
446da04
3df9796
f56ccec
6cd046d
69c7998
7b2e368
26eb83c
a4bd79b
809dbdc
9150216
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not sure why this has changed. It looks like the radios at the bottom with the dropdown have changed slightly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@precious-onyenaucheya-ons have you looked in to this one? Doesn't look major but curious to see if there is a reason for the change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@admilne @rmccar I am not sure why this changed, I have not made any change in this ticket that will affect this pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one needs investigating
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to need important. If we can we should avoid using important and only use it if we need to. Can you review the places youve added important and check that they are needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rmccar I see this hasn't changed. It looks like it is required at the moment because there is some css later on in the file which overrides the padding needed here. On line
780
in_button.scss
you have the following:This looks a little odd to me, anyone know if this is required? But this is the style resulting in @precious-onyenaucheya-ons needing to use
!important
. It's more specific and comes later in the fileThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code was to align the height of the button when it is used in places like this:
https://service-manual.ons.gov.uk/design-system/components/input/example-input-search
Before this the button shadow would sit below the input.
This seems to be fine when important is removed. Have you seen any issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rmccar yeah, it's subtle but the button isn't quite as wide without the important tag. I can't remember off the top of my head but I think the icon perhaps wasn't quite centre either?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? This line seems to make no difference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@precious-onyenaucheya-ons I don't think this comment has been addressed yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@precious-onyenaucheya-ons I think this is still in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look like its been removed to me either