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

Fix the small padding issue #2836

Merged
merged 7 commits into from Nov 16, 2023
Merged

Fix the small padding issue #2836

merged 7 commits into from Nov 16, 2023

Conversation

Rithvik-padma
Copy link
Contributor

Describe the changes you have made in this PR

fix the small padding issue for confirm/cancel buttons in the LNURL pay screen when the "more options" button is clicked

Link this PR to an issue [optional]

Fixes #2788

Type of change

  • fix: Bug fix (non-breaking change which fixes an issue)

Screenshots of the changes [optional]

VvcZLQbvCc

How has this been tested?

Tested manually

Checklist

  • Self-review of changed code
  • Manual testing
  • Responsive layout

@Rithvik-padma
Copy link
Contributor Author

@rolznz can u please review this PR.
Thanks!

@pavanjoshi914
Copy link
Contributor

pavanjoshi914 commented Nov 13, 2023

this will always break in the future for other screens where we use show more button. better to make a permanent fix here!

the problem here is when we expand fields, the bottom padding present in container field is not reflecting.

an alternate to fix this would be to assign bottom paddings to the last button div itself and not in the container.tsx (https://github.com/getAlby/lightning-browser-extension/blob/master/src/app/components/Container.tsx#L32 notice pb-4 here) so that this will not be issue for every screen which has expnading fields

but this will lead to changes in other files where the container component is used.

wdyt @rolznz

@Rithvik-padma
Copy link
Contributor Author

this will always break in the future for other screens where we use show more button. better to make a permanent fix here!

the problem here is when we expand fields, the bottom padding present in container field is not reflecting.

an alternate to fix this would be to assign bottom paddings to the last button div itself and not in the container.tsx (https://github.com/getAlby/lightning-browser-extension/blob/master/src/app/components/Container.tsx#L32 notice pb-4 here) but this will lead to changes in other files where the container component is used.

wdyt @rolznz

Yes I agree, some change has to be made in Container component for a permanent fix or change the button padding everywhere Container component is used

@rolznz
Copy link
Contributor

rolznz commented Nov 14, 2023

@pavanjoshi914 @Rithvik-padma I noticed in the Container component there is a h-full class which is problematic if the height of the component becomes larger than the height of the popup because the bottom will be cut off (here you see the padding, but it's actually worse than that). It seems to fix it if this is changed to min-h-full without any need to change padding elsewhere and I can't see any side effects. Does this look good to you? CC @reneaaron

@Rithvik-padma
Copy link
Contributor Author

@pavanjoshi914 @Rithvik-padma I noticed in the Container component there is a h-full class which is problematic if the height of the component becomes larger than the height of the popup because the bottom will be cut off (here you see the padding, but it's actually worse than that). It seems to fix it if this is changed to min-h-full without any need to change padding elsewhere and I can't see any side effects. Does this look good to you? CC @reneaaron

@rolznz I have already tried doing that but the alignment of the child components is getting disturbed in the this screen and its possible to happen in other screens where Container component is being used as the alignment is done considering the h-full class in the Container component. The LNURL pay screen looks like this if min-h-full instead of the h-full.

image

@rolznz
Copy link
Contributor

rolznz commented Nov 15, 2023

@Rithvik-padma can you highlight the issue in the screenshot? is it the extra spacing before the description? if you inspect it, now the css rules (like justify-between) on the container are correctly being applied and you can see the first element is just the publisher card.

I think the fix I suggested is now revealing an existing issue. If we don't want spacing there, we should not be using justifyBetween to add space between the publisher card and the form

@Rithvik-padma
Copy link
Contributor Author

@Rithvik-padma can you highlight the issue in the screenshot? is it the extra spacing before the description? if you inspect it, now the css rules (like justify-between) on the container are correctly being applied and you can see the first element is just the publisher card.

I think the fix I suggested is now revealing an existing issue. If we don't want spacing there, we should not be using justifyBetween to add space between the publisher card and the form

@rolznz So I just found out that the possible reason for this issue is the fieldset element, it is not aligning as expected for flex, I have tried using the div tag instead of it and it works fine by considering min-h-full class in Container component.

So ig this means that there is no breaking if we use the min-h-full class in the Container component but the problem is with the fieldset tag.

I will make some changes for this issue by replacing the fieldset tag, can you please take a look into it.

@rolznz
Copy link
Contributor

rolznz commented Nov 15, 2023

@Rithvik-padma the send page looks good to me. The close button on the success page is a bit broken - could you please check that one?
image

@Rithvik-padma
Copy link
Contributor Author

@Rithvik-padma the send page looks good to me. The close button on the success page is a bit broken - could you please check that one?

image

Sure!

@Rithvik-padma
Copy link
Contributor Author

Rithvik-padma commented Nov 15, 2023

@rolnz how can I test the success page while using a LND test account

@Rithvik-padma
Copy link
Contributor Author

@rolznz I have fixed the padding issue in the result page, apparently min-h-full class was causing an issue there.

Copy link
Contributor

@rolznz rolznz left a comment

Choose a reason for hiding this comment

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

tACK

@rolznz rolznz merged commit 9cd7645 into getAlby:master Nov 16, 2023
6 of 7 checks passed
@rolznz
Copy link
Contributor

rolznz commented Nov 16, 2023

Thanks @Rithvik-padma !

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.

[BUG] Small padding issue for the confirm/cancel buttons in the LNURL pay send screen
3 participants