Ticket #598 updated #639

zarSou9 · 2024-05-25T20:26:12Z

Unlike the previous pr for this ticket, this one uses the Action component. I've created a ButtonSecondary component as a smaller alternative to the Button component with slightly different styling as to match the Figma.

tailwindcss was added to improve the styling experience

Note: All 3 imports in the root.css file are needed for tailwind to work correctly. Also turns out the reference problems were happening cus I was using pnpm.

mruwnik · 2024-05-25T21:45:37Z

app/routes/questions.actions.tsx

  hint?: string
-  children?: ReactNode | ReactNode[]


keep the children. This allows extra components to be provided

You right, I added it back

mruwnik · 2024-05-25T21:52:43Z

app/routes/questions.actions.tsx

  hint?: string
-  children?: ReactNode | ReactNode[]
+  dissabled?: boolean


mruwnik · 2024-05-25T21:57:57Z

app/components/ButtonSecondary/button_secondary.css

@@ -0,0 +1,15 @@
+.button-secondary .tool-tip {


isn't the whole point of tailwind to avoid having extra css?

Tailwind does have some limitations. i.e. it is difficult to use selectors to control the styling of child elements.

mruwnik · 2024-05-25T22:01:43Z

app/components/Feedback/index.tsx

-    return () => clearInterval(timeout)
-  }, [showFeedback])
+    if (showThanks && thanksRef.current) {
+      thanksRef.current.style.opacity = '1'


use classes for css. Or contitional rendering if you want to use js. Don't mix the two - there lies madness

why this way, rather than the previous approach?

You right on both accounts. I've updated the code

mruwnik · 2024-05-25T22:03:37Z

app/components/Feedback/index.tsx

        <Action
          pageid={pageid}
          showText={!!labels}
          actionType={ActionType.HELPFUL}
+          active={vote === 'helpful'}
+          dissabled={!!vote}


mruwnik · 2024-05-25T22:05:40Z

app/components/Feedback/index.tsx

  const [showFeedbackForm, setShowFeedbackForm] = useState(false)
+  const [vote, setVote]: any = useState(undefined)


this means that each time the page gets refreshed, the user can vote for the article again. The previous solution would rememeber whether an article was liked or not between refreshes

very good point. I've updated the code, but note that I had to pass a setVoted function to the child Action components so that the feedback knew to set the other Action disabled. Its the best I could do. Also the handler function for voting in the questions.actions file is throwing an error, and thus the feedback button is returning to its null state after you press it.

zarSou9 · 2024-05-26T03:38:32Z

I've incorporated all of your feedback except the check is failing for absolutely now reason. A prettier error even though I have run prettier. Oh well, I'm going to sleep

zarSou9 · 2024-05-29T02:42:57Z

Ok, no more tailwind, should be ready to merge 🤞

mruwnik · 2024-06-01T10:18:55Z

app/components/Article/article.css

 article .footer-comtainer {
  display: flex;
  align-items: center;
  margin-top: var(--spacing-56);
  margin-bottom: var(--spacing-24);
+  gap: 16px;


try to avoid raw pixel values. In root.css there's a bunch of available variables that should be used for sizing and coiours. You use them as var(<var name), like in the previous lines. So in this case it'd be gap: var(--spacing-16);. This is important both for consistency (as it's easier to change everything at once), but also because the mobile version often has subtle differences in spacing which should be reflected by this

app/components/ButtonSecondary/index.tsx

mruwnik · 2024-06-01T10:29:46Z

app/components/Feedback/feedback.css

 }

 /* Comment to Nemo when he reviews: why didn't you just do this the way you did the settings buttons? */
 .select-option {
  height: var(--spacing-48);
-  padding: var(--spacing-8) var(--spacing-16);
+  border-radius: 6px;


use a var here. Or consider adding the rounded class to whatever is using this

yes I've changed it. The only reason I used the 6px is because that's what the figma said, but the var for --border-radius is 4px. I totally get the value of only needing to change one variable though so yeah.

app/components/Feedback/feedback.css

mruwnik · 2024-06-01T10:30:26Z

app/components/Feedback/feedback.css

+
+.submit {
+  height: var(--spacing-48);
+  border-radius: 6px;


There's actually a variable for border radius so use that var!

mruwnik · 2024-06-01T10:31:13Z

app/components/Feedback/feedback.css

@@ -28,7 +54,6 @@
  padding: var(--spacing-12);
  margin: var(--spacing-40) 0;
  box-sizing: content-box;
-  background: pink;


hmm. interesting...

mruwnik · 2024-06-01T10:32:05Z

app/components/Feedback/index.tsx


-      {showFeedback && <div className="thanks">Thanks for your feedback!</div>}
+      <p className={'thanks ' + (showThanks ? 'show' : 'hide')}>Thank you for your feedback!</p>


out of curiosity, why are you doing it this way?

because I wanted a css fade in/out transition which can't be done easily with conditional rendering to my knowledge. (in svelte you can tho 🙂)

mruwnik · 2024-06-01T10:32:42Z

app/components/Feedback/index.tsx


      {showFeedbackForm && (
        <FeedbackForm
-          onSubmit={onSubmit}
+          onSubmit={(att) => {


can you name this something more intuitive?

mruwnik · 2024-06-01T10:33:08Z

app/root.css

@@ -160,6 +160,9 @@ h2 {
  letter-spacing: -0.14px;
 }

+.leading-0 {
+  line-height: 0 !important;


@melissasamworth

When I was trying to style the button, It kept putting this annoying space below it. After some testing, I discovered it was a line-height issue. I've added this to the root incase there are other circumstances where there is a line-height problem.

Fine with me

Except sometimes the button actually goes to two lines. In which case this would end up looking really weird and bad obviously.

mruwnik · 2024-06-01T10:34:32Z

app/routes/questions.actions.tsx

@@ -173,13 +184,9 @@ export const Action = ({
      <input type="hidden" name="incBy" value={actionTaken ? -1 : 1} />
      <input type="hidden" name="stateString" value={stateString} />
      {children}
-      <Button className={className}>
+      <ButtonSecondary disabled={disabled} active={actionTaken} tooltip={hint}>


this is also just a normal button with a custom tooltip

…to 598-updated

mruwnik

@melissasamworth can you check the css bits? The code seems fine

mruwnik · 2024-06-01T16:58:52Z

app/components/Button/button.css

+}
+
+.active {
+  background-color: #edfaf9;


these should also be vars and use the colours in root.css

mruwnik · 2024-06-01T17:02:14Z

app/components/Button/button.css

@@ -20,6 +20,15 @@
  letter-spacing: -0.16px;
 }

+.button-secondary {


how does this differ from the .secondary class below? That is conceptually what's the difference?

The second version of the button, as represented in the figma, is significantly stylistically different, mainly as a result of the difference in hover states (the main button shows and outline, and the second shows a background). This also effects how sibling buttons are grouped. The .secondary class is a slight difference to the main button version, and the .button-secondary class is a second version of the button.

app/components/Feedback/Form.tsx

app/components/Article/index.tsx

app/components/ButtonSecondary/index.tsx

melissasamworth · 2024-06-04T00:23:41Z

app/root.css

@@ -160,6 +160,9 @@ h2 {
  letter-spacing: -0.14px;
 }

+.leading-0 {
+  line-height: 0 !important;


Fine with me

melissasamworth · 2024-06-04T00:24:04Z

app/root.css

@@ -160,6 +160,9 @@ h2 {
  letter-spacing: -0.14px;
 }

+.leading-0 {
+  line-height: 0 !important;


Except sometimes the button actually goes to two lines. In which case this would end up looking really weird and bad obviously.

melissasamworth · 2024-06-04T00:25:31Z

app/components/Feedback/feedback.css

+
+.submit {
+  height: var(--spacing-48);
+  border-radius: 6px;


There's actually a variable for border radius so use that var!

app/components/Feedback/feedback.css

zarSou9

testing review

fixing review error

zarSou9 added 3 commits May 25, 2024 16:01

Feedback updated, ButtonSecondary added

3338bf5

Fixed reference errors by switching to npm

d211cf2

Fixed linting error

6224504

zarSou9 requested a review from mruwnik May 25, 2024 21:17

zarSou9 added 3 commits May 25, 2024 17:22

added node/types

bc3bb58

Hopefully fixed test err

9ce820c

Fixed type errors

0a3dca9

mruwnik reviewed May 25, 2024

View reviewed changes

zarSou9 added 4 commits May 25, 2024 22:51

save feedback to local storage

f38104c

fixed js css interaction

9b394ea

linting issue fix

488843b

prettier is acting weird

3510b72

zarSou9 added 5 commits May 26, 2024 10:25

correct prettier file

e8cc39b

prettier is not pretty

90c0f4c

prettier needs to die

a96a9eb

idk anymore

519ac26

Merge remote-tracking branch 'upstream/stampy-redesign' into 598-updated

bd231aa

zarSou9 requested a review from melissasamworth May 27, 2024 22:20

zarSou9 and others added 3 commits May 28, 2024 21:54

removed tailwind

1c52886

form styling

efc5302

Merge branch 'stampy-redesign' into 598-updated

932b918

mruwnik reviewed Jun 1, 2024

View reviewed changes

zarSou9 added 2 commits June 1, 2024 12:54

Incorporated feedback, Deleted ButtonSecondary

282a58b

Merge branch '598-updated' of https://github.com/zarSou9/stampy-ui in…

07e37bd

…to 598-updated

mruwnik reviewed Jun 1, 2024

View reviewed changes

color vars added

3f7bdbe

melissasamworth previously requested changes Jun 4, 2024

View reviewed changes

vars added, position form absolutely

a571c1b

linting

f8ee121

zarSou9 assigned melissasamworth Jun 4, 2024

zarSou9 commented Jun 4, 2024

View reviewed changes

melissasamworth approved these changes Jun 4, 2024

View reviewed changes

melissasamworth merged commit dc6a47c into StampyAI:stampy-redesign Jun 4, 2024
1 check passed

		const [showFeedbackForm, setShowFeedbackForm] = useState(false)
		const [vote, setVote]: any = useState(undefined)


		{showFeedback && <div className="thanks">Thanks for your feedback!</div>}
		<p className={'thanks ' + (showThanks ? 'show' : 'hide')}>Thank you for your feedback!</p>

Ticket #598 updated #639

Ticket #598 updated #639

Conversation

zarSou9 commented May 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zarSou9 commented May 26, 2024

zarSou9 commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mruwnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zarSou9 left a comment

Choose a reason for hiding this comment

zarSou9 commented May 25, 2024 •

edited

Loading